-
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
fix: disambiguate method names that are reserved in transport classes #1187
Conversation
In addition to the method specific stubs they provide, the generated transports expose other methods to e.g. create a gRPC channel. This presents the opportunity for a naming collision if an API has a CreateChannel method. This PR disambiguates colliding method names at the transport level. Client level method names are unchanged for ergonomic reasons.
ae9a5c8
to
8772819
Compare
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.
LGTM
# | ||
# Note: this should really be a class variable, | ||
# but python 3.6 can't handle that. | ||
TRANSPORT_UNSAFE_NAMES = { |
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.
Looks like it is the second in a row disambiguating PR (the other one: #1178). Can we (should we) have some sort of centralized name-conflict resolution mechanism? I think it is fine for this PR to be as is, but in case more things like that happen, it would be good to have a unified name-conflict resolution unility class or something like that. WDYT?
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.
Different names that refer to different data structures need to be disambiguated against different sets of names. It doesn't matter too much if there's a field named create_channel
. I'd argue that there are costs to being too aggressive disambiguating names, and that it makes more sense to deal with them in the Method
or Field
class when collisions occur.
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.
I agree that they have to be disamgiguated against different sets of names, but that set of names can be a standardized parameter to the generic disambiguating mechanism. Basically some mechanism though which all names go through, and get disambiguated depending on the context. But yeah, this is beyond the scope of this PR and we have many more important tasks to work on.
🤖 I have created a release *beep* *boop* --- ### [0.63.1](v0.63.0...v0.63.1) (2022-02-03) ### Bug Fixes * disambiguate method names that are reserved in transport classes ([#1187](#1187)) ([78626d8](78626d8)) --- This PR was generated with [Release Please](https://github.com/googleapis/release-please). See [documentation](https://github.com/googleapis/release-please#release-please).
In addition to the method specific stubs they provide, the generated
transports expose other methods to e.g. create a gRPC channel.
This presents the opportunity for a naming collision if an API has a
CreateChannel method.
This PR disambiguates colliding method names at the transport
level. Client level method names are unchanged for ergonomic reasons.