Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

feat: Support numeric enums: BUILD file updates, new Go BUILD files #111

Merged
merged 7 commits into from
Sep 1, 2022

Conversation

vchudnov-g
Copy link
Contributor

@vchudnov-g vchudnov-g commented Aug 26, 2022

This commit contains two changes:

  1. When updating BUILD files, any pre-existing values of the
    rest_numeric_enums parameter will be preserved. This feature will
    be enabled for each language as they add support for that parameter
    in BUILD.bazel.gapic_api.mustache, which they'll typically do in
    separate CLs once they implement the parameter in their BUILD rule
    definition.

  2. We also add support for rest_numeric_enums in
    BUILD.bazel.gapic_api.mustache for Go, since that will be the first
    language where the feature will be rolled out.

As much as we would have preferred to have these be separate commits,
they are entangled by the fact that testing (1) requires changes to
the template for one language, which is why we are doing (2) at the
same time.

This commit contains two changes:

1. When updating BUILD files, any pre-existing values of the
   `rest_numeric_enums` parameter will be preserved. This feature will
   be enabled for each language as they add support for that parameter
   in BUILD.bazel.gapic_api.mustache, which they'lltypically do in
   separate CLs once they implement the parameter in their BUILD rule
   definition.

2. We also add support for `rest_numeric_enums` in
   BUILD.bazel.gapic_api.mustache for Go, since that will be the first
   language where the feature will be rolled out.

As much as we would have preferred to have these be separate commits,
they are entangled by the fact that testing (1) requires changes to
the template for one language, which is why we are doing (2) at the
same time.
@vchudnov-g vchudnov-g requested a review from a team as a code owner August 26, 2022 01:48
// Multiple languages:
PRESERVED_PROTO_LIBRARY_STRING_ATTRIBUTES.put("package_name", null);
PRESERVED_PROTO_LIBRARY_STRING_ATTRIBUTES.put("transport", null);
PRESERVED_PROTO_LIBRARY_STRING_ATTRIBUTES.put("rest_numeric_enums", "False");
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

would it be a string "False", or a boolean False? I feel there is a possibility of an inconsistency here.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Or, if I'm guessing correctly, for a string, it should be "\"False\""?

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

After looking at the remaining code, I must say it is still unclear to me. Would it be possible to inspect the actual content of the build file that had False value added? Something tells me that it would be a string "False" rather than a boolean.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is intended to be a boolean value as per the internal design doc, which is reflected in the Go and Python implementations. For sample files, you can look at the mustache and baseline files in this PR. Or were you looking for something else?

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Definitely a boolean False.

Copy link
Member

@codyoss codyoss left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM

Copy link
Contributor

@noahdietz noahdietz left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM from Go side

Copy link
Contributor

@vam-google vam-google left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM

This helps ensure that updated parameters are quoted or not, as expected.

Note that with this commit, updated values of rest_numeric_enums are quoted, which is incorrect.
TODO next: Fix this
The renaming makes it explicit they operate on string values.
@vchudnov-g
Copy link
Contributor Author

vchudnov-g commented Sep 1, 2022

The "overrides" (i.e. preserving previous values in the BUILD file) were always inserting values as strings. Fixed that and added tests. PTAL.

Copy link
Contributor

@alexander-fenster alexander-fenster left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM, thank you for fixing the non-string case!

@vchudnov-g vchudnov-g merged commit 736334d into main Sep 1, 2022
@vchudnov-g vchudnov-g deleted the numeric_enum_upgrade_go branch September 1, 2022 19:13
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

5 participants