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: freezes reserved names list. #1575

Merged
merged 7 commits into from
Mar 28, 2023
Merged

Conversation

atulep
Copy link
Contributor

@atulep atulep commented Jan 23, 2023

This PR implements a solution proposed to reduce number of breaking changes occurring in downstream GAPIC libraries. It addresses the common denominator between #835, googleapis/python-video-transcoder#121 (comment) and googleapis/python-dataflow-client#143.

Concretely, this PR freezes the list of reserved names. The list values come from a careful analysis on Google APIs. Specifically, the old list was filtered based on its usage in Google APIs protos: if the value was used in any of the API protos, that value was retained in the new list. This ensures backwards compatibility.

@product-auto-label product-auto-label bot added the size: m Pull request size is medium. label Jan 23, 2023
@product-auto-label product-auto-label bot added size: l Pull request size is large. and removed size: m Pull request size is medium. labels Jan 24, 2023
@atulep atulep marked this pull request as ready for review January 24, 2023 22:52
@atulep atulep requested review from a team as code owners January 24, 2023 22:52
@product-auto-label product-auto-label bot added size: m Pull request size is medium. and removed size: l Pull request size is large. labels Jan 25, 2023
@parthea parthea self-assigned this Feb 3, 2023
@atulep
Copy link
Contributor Author

atulep commented Feb 9, 2023

@parthea PTAL

Comment on lines -15 to -17
import builtins
import itertools
import keyword
Copy link
Contributor

Choose a reason for hiding this comment

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

Please could you also check other usages of keywords in the code?

RESERVED_WORDS = frozenset(
itertools.chain(
keyword.kwlist,
dir(__builtins__),

# "metadata", "retry", "timeout", and "request" are reserved words in client methods.
invalid_module_names = set(keyword.kwlist) | {
"metadata", "retry", "timeout", "request"}

@property
def safe_name(self) -> str:
# Used to prevent collisions with python keywords at the client level
name = self.name
return name + "_" if name.lower() in keyword.kwlist else name

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Thanks for the find.

  • gapic-generator-python/gapic/schema/api.py
    This is not related to protoplus reserved names. The keyword list is included to avoid syntax errors from Python and "metadata" etc are added because these are "reserved" inside of client methods. For example, "request" is hardcoded in templates as an argument to all client methods.
  • gapic-generator-python/gapic/schema/wrappers.py this seems to only deal with Python keywords for the same reason as above. Again, unrelated to protoplus reserved names.

samplegen looks legit. Looking into it.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

RESERVED_WORDS in samplegen also doesn't relate to protoplus. Their list is used in

def _handle_lvalue(self, lval: str, type_: wrappers.Field):
"""Conducts safety checks on an lvalue and adds it to the lexical scope.
Raises:
ReservedVariableName: If an attempted lvalue is a reserved keyword.
"""
if lval in RESERVED_WORDS:
raise types.ReservedVariableName(
"Tried to define a variable with reserved name: {}".format(
lval)
)
, where they try to generate sample code, and want to avoid invalid Python. Actually, their list is also too big (e.g. builtins not need to be there), but it's a separate problem.

@parthea parthea assigned atulep and unassigned parthea Mar 6, 2023
@atulep atulep requested a review from parthea March 9, 2023 22:10
@atulep atulep assigned parthea and unassigned atulep Mar 20, 2023
@atulep
Copy link
Contributor Author

atulep commented Mar 22, 2023

Spoke with Tony, and he wanted to run some internal diagnostics before LGTM, but got busy with other tasks. I'll run them on his behalf.

@atulep
Copy link
Contributor Author

atulep commented Mar 28, 2023

There is no diff between generated client libs with these changes applied and the main branch.

@parthea parthea added the owlbot:run Add this label to trigger the Owlbot post processor. label Mar 28, 2023
@gcf-owl-bot gcf-owl-bot bot removed the owlbot:run Add this label to trigger the Owlbot post processor. label Mar 28, 2023
@parthea parthea merged commit b1b56ab into googleapis:main Mar 28, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
size: m Pull request size is medium.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants