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: Enable pytype on Pub/Sub Lite repo and fix all errors #214
Conversation
google/cloud/pubsublite/cloudpubsub/internal/single_publisher.py
Outdated
Show resolved
Hide resolved
"subscription_id": path.name, | ||
"skip_backlog": (starting_offset == BacklogLocation.END), | ||
} | ||
request=CreateSubscriptionRequest( |
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.
This is potentially a breaking behavior change due to some known issues in proto-plus if you have any users that have passed in a subscription as a dictionary
googleapis/proto-plus-python#161 comes to mind, but I think there was another one specifically about mixing dictionaries and protos, but I'm having trouble finding 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.
Found it! googleapis/python-tasks#148
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.
Acknowledged. I'm okay with this- the type hints tell the user the exact type they need to pass. I don't think this library has wide enough usage that its expected to be bug-compatable with past versions: this is only a breaking change if proto-plus-python has a bug preventing usage of a dict where it should be usable, and the user is ignoring the type hint provided on the method.
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 haven't looked closely enough to know whether it's a breaking change or not, but if it is, we should update the issue title to not use chore
accordingly.
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.
Done.
Merge-on-green attempted to merge your PR for 6 hours, but it was not mergeable because either one of your required status checks failed, one of your required reviews was not approved, or there is a do not merge label. Learn more about your required status checks here: https://help.github.com/en/github/administering-a-repository/enabling-required-status-checks. You can remove and reapply the label to re-run the bot. |
🤖 I have created a release \*beep\* \*boop\* --- ### [1.1.2](https://www.github.com/googleapis/python-pubsublite/compare/v1.1.1...v1.1.2) (2021-09-13) ### Bug Fixes * Enable pytype on Pub/Sub Lite repo and fix all errors ([#214](https://www.github.com/googleapis/python-pubsublite/issues/214)) ([df58fdf](https://www.github.com/googleapis/python-pubsublite/commit/df58fdfc83bdc4f6f753f664365a0ff26d3201e7)) * performance issues with subscriber client ([#232](https://www.github.com/googleapis/python-pubsublite/issues/232)) ([78a47b2](https://www.github.com/googleapis/python-pubsublite/commit/78a47b2817bee4a468f8ce15fe437165be1d1458)) --- This PR was generated with [Release Please](https://github.com/googleapis/release-please). See [documentation](https://github.com/googleapis/release-please#release-please).
Workarounds for google/pytype#977 and google/pytype#978 are present in this PR, as well as a backport of googleapis/gapic-generator-python#970
Also modifies construction of CreateSubscriptionRequests slightly