-
Notifications
You must be signed in to change notification settings - Fork 1.5k
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
Pub/Sub: staticmethod check #8091
Conversation
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.
On the surface the fix looks good, the straightforward isinstance
checks make the logic indeed more readable. 👍 Gave two suggestions for improvement.
I noticed, though, that we currently do not have any unit tests for the _gapic.add_methods()
decorator, which could have prevented the discovered issue with static methods. Since already touching this part of the code, it represents a good chance to fill this gap and improve test coverage.
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.
The code LGTM now.
What about the tests, are they in the making?
Let me see if the test can prove that the code really works. I will add them next. Thanks for the quick turnaround! |
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.
The tests looks good!
Just have two naming/comment suggestions, but otherwise lean towards merging this.
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 good now, merging. 👍
I don't have permission to merge. If you do, please do! :) |
The GAPIC wrapper (code snippet below with comments) in
pubsub/google/cloud/pubsub_v1/_gapic
falsely recognizes static methods as instance methods.The PR adds a check for static methods and aims to make the code more readable.