-
Notifications
You must be signed in to change notification settings - Fork 69
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
Conversation
@parthea PTAL |
import builtins | ||
import itertools | ||
import keyword |
There was a problem hiding this comment.
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?
gapic-generator-python/gapic/samplegen/samplegen.py
Lines 44 to 47 in 13b6c6c
RESERVED_WORDS = frozenset( | |
itertools.chain( | |
keyword.kwlist, | |
dir(__builtins__), |
gapic-generator-python/gapic/schema/api.py
Lines 269 to 271 in 13b6c6c
# "metadata", "retry", "timeout", and "request" are reserved words in client methods. | |
invalid_module_names = set(keyword.kwlist) | { | |
"metadata", "retry", "timeout", "request"} |
gapic-generator-python/gapic/schema/wrappers.py
Lines 1141 to 1146 in 13b6c6c
@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 |
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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
gapic-generator-python/gapic/samplegen/samplegen.py
Lines 710 to 720 in 13b6c6c
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) | |
) |
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. |
There is no diff between generated client libs with these changes applied and the main branch. |
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.