refactor: Json to proto message refactoring #2085

arturowczarek · 2023-04-22T09:41:25Z

Thank you for opening a Pull Request! Before submitting your PR, there are a few things you can do to make sure it goes smoothly:

Make sure to open an issue as a bug/issue before writing your code! That way we can discuss the change, evaluate designs, and agree on the general idea
Ensure the tests and linter pass
Code coverage does not decrease (if any source code was changed)
Appropriate docs were updated (if necessary)

Changes:
refactor: Use equalsIgnoreCase for JSON string boolean values in JsonToProtoMessage and use primitives for booleans
refactor: Use enhanced for in JsonToProtoMessage
refactor: Properly reference to static field INSTANCE in JsonToProtoMessage
refactor: Remove unnecessary LOG field from JsonToProtoMessage
refactor: Remove unnecessary casts from JsonToProtoMessage methods and remove usage of soon deprecated (Java 9) Long constructor
refactor: Remove unnecessary variables "message" from JsonToProtoMessage methods
refactor: Remove topLevel parameter from JsonToProtoMessage methods
refactor: Extract magic variables to constants
refactor: Capitalize deep constants in JsonToProtoMessage and make them final

Fixes #2042 ☕️

If you write sample code, please follow the samples format.

yirutang · 2023-04-26T17:27:34Z

...d-bigquerystorage/src/main/java/com/google/cloud/bigquery/storage/v1/JsonToProtoMessage.java

@@ -102,8 +99,10 @@ public class JsonToProtoMessage implements ToProtoConverter<Object> {
          .optionalEnd()
          .toFormatter()
          .withZone(ZoneOffset.UTC);
+  private final String ROOT_JSON_SCOPE = "root";
+  private final boolean DONT_ACCEPT_UNKNOWN_FIELDS = false;


It is a bit strange to have this constant. I think it is easier to read just to keep the bollean.

Are you sure? It explicitly names the intention. Otherwise you may mix boolean values:
return convertToProtoMessage(protoSchema, null, json, "root", true, false);
Can you tell what is true and false here? Your IDE will probably help you here, but what about GitHub review? Are you sure you won't put topLevel value in place of ignoreUnknownArguments parameter?

Usually we will just annotate the parameters with the argument name. So it is just:
protoSchema,
tableSchema.getFieldsList(),
json,
"root",
/topLevel=/ true,
/ignoreUnknownFields/ false);

Do you want me to inline only DONT_ACCEPT_UNKNOWN_FIELDS or ROOT_JSON_SCOPE too?
What IDE do you use? I thought all of them display the names of the parameters where constants are used. See the screenshot:

DONT_ACCEPT_UNKNOWN_FIELDS and ROOT_JSON_SCOPE. Thanks.

yirutang · 2023-04-26T17:27:54Z

...d-bigquerystorage/src/main/java/com/google/cloud/bigquery/storage/v1/JsonToProtoMessage.java

@@ -25,13 +25,11 @@
 import com.google.protobuf.Descriptors.Descriptor;


Thanks for improving the code!
Could you provide a list of things refactored in the PR description?

…em final

…age methods

…d remove usage of soon deprecated (Java 9) Long constructor

…essage

…ToProtoMessage and use primitives for booleans

…sonToProtoMessage repeated fields.

… in JsonToProtoMessage

See https://github.com/googleapis/repo-automation-bots/blob/main/packages/owl-bot/README.md

arturowczarek requested a review from a team as a code owner April 22, 2023 09:41

arturowczarek requested a review from Neenu1995 April 22, 2023 09:41

product-auto-label bot added size: m Pull request size is medium. api: bigquerystorage Issues related to the googleapis/java-bigquerystorage API. labels Apr 22, 2023

arturowczarek changed the title ~~Json to proto message refactoring~~ refactor: Json to proto message refactoring Apr 22, 2023

yirutang reviewed Apr 26, 2023

View reviewed changes

Neenu1995 added kokoro:force-run Add this label to force Kokoro to re-run the tests. owlbot:run Add this label to trigger the Owlbot post processor. labels May 1, 2023

gcf-owl-bot bot removed the owlbot:run Add this label to trigger the Owlbot post processor. label May 1, 2023

yoshi-kokoro removed the kokoro:force-run Add this label to force Kokoro to re-run the tests. label May 1, 2023

gcf-owl-bot bot requested a review from a team as a code owner May 1, 2023 17:25

Neenu1995 mentioned this pull request May 1, 2023

fix: pass the parameter value of enableConnectionPool instead of true always #2096

Merged

arturowczarek force-pushed the json_to_proto_message_refactoring branch from b832cd7 to abea73c Compare May 2, 2023 06:15

arturowczarek added 10 commits May 2, 2023 08:16

refactor: Capitalize deep constants in JsonToProtoMessage and make th…

f1a12fa

…em final

refactor: Extract magic variables to constants

1c54343

refactor: Remove topLevel parameter from JsonToProtoMessage methods

8255df4

refactor: Remove unnecessary variables "message" from JsonToProtoMess…

51084c8

…age methods

refactor: Remove unnecessary casts from JsonToProtoMessage methods an…

ec19dfa

…d remove usage of soon deprecated (Java 9) Long constructor

refactor: Remove unnecessary LOG field from JsonToProtoMessage

04ddc0c

refactor: Properly reference to static field INSTANCE in JsonToProtoM…

bddb2c7

…essage

refactor: Use enhanced for in JsonToProtoMessage

5e49f04

refactor: Use equalsIgnoreCase for JSON string boolean values in Json…

76db7ee

…ToProtoMessage and use primitives for booleans

refactor: Replace fail variable with failure extracted to method in J…

4aad60d

…sonToProtoMessage repeated fields.

arturowczarek force-pushed the json_to_proto_message_refactoring branch from abea73c to ef24980 Compare May 2, 2023 06:16

arturowczarek requested a review from yirutang May 2, 2023 06:17

refactor: Inline json scope variable and ignoreUnknownFields variable…

009ced0

… in JsonToProtoMessage

arturowczarek force-pushed the json_to_proto_message_refactoring branch from ef24980 to 009ced0 Compare May 2, 2023 07:35

Neenu1995 added kokoro:force-run Add this label to force Kokoro to re-run the tests. owlbot:run Add this label to trigger the Owlbot post processor. labels May 3, 2023

gcf-owl-bot bot removed the owlbot:run Add this label to trigger the Owlbot post processor. label May 3, 2023

yoshi-kokoro removed the kokoro:force-run Add this label to force Kokoro to re-run the tests. label May 3, 2023

🦉 Updates from OwlBot post-processor

530ec67

See https://github.com/googleapis/repo-automation-bots/blob/main/packages/owl-bot/README.md

Neenu1995 added the kokoro:force-run Add this label to force Kokoro to re-run the tests. label May 3, 2023

yoshi-kokoro removed the kokoro:force-run Add this label to force Kokoro to re-run the tests. label May 3, 2023

yirutang approved these changes May 3, 2023

View reviewed changes

yirutang merged commit bcd7fc9 into googleapis:main May 3, 2023

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

refactor: Json to proto message refactoring #2085

refactor: Json to proto message refactoring #2085

arturowczarek commented Apr 22, 2023 •

edited

Loading

yirutang Apr 26, 2023

arturowczarek Apr 27, 2023

yirutang Apr 27, 2023

arturowczarek Apr 27, 2023

yirutang May 1, 2023

arturowczarek May 2, 2023

yirutang Apr 26, 2023

arturowczarek Apr 27, 2023

		@@ -25,13 +25,11 @@
		import com.google.protobuf.Descriptors.Descriptor;

refactor: Json to proto message refactoring #2085

refactor: Json to proto message refactoring #2085

Conversation

arturowczarek commented Apr 22, 2023 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

arturowczarek commented Apr 22, 2023 •

edited

Loading