Skip to content

Conversation

@dpcollins-google
Copy link
Collaborator

@dpcollins-google dpcollins-google commented Aug 16, 2021

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

@dpcollins-google dpcollins-google requested review from a team as code owners August 16, 2021 16:41
@google-cla google-cla bot added the cla: yes This human has signed the Contributor License Agreement. label Aug 16, 2021
@product-auto-label product-auto-label bot added the api: pubsublite Issues related to the googleapis/python-pubsublite API. label Aug 16, 2021
"subscription_id": path.name,
"skip_backlog": (starting_offset == BacklogLocation.END),
}
request=CreateSubscriptionRequest(
Copy link

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.

Copy link

Choose a reason for hiding this comment

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

Copy link
Collaborator Author

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.

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.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Done.

@dpcollins-google dpcollins-google added the automerge Merge the pull request once unit tests and other checks pass. label Aug 18, 2021
@gcf-merge-on-green
Copy link

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.

@gcf-merge-on-green gcf-merge-on-green bot removed the automerge Merge the pull request once unit tests and other checks pass. label Aug 18, 2021
@dpcollins-google dpcollins-google changed the title chore: Enable pytype on Pub/Sub Lite repo and fix all errors fix: Enable pytype on Pub/Sub Lite repo and fix all errors Sep 13, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

api: pubsublite Issues related to the googleapis/python-pubsublite API. cla: yes This human has signed the Contributor License Agreement.

5 participants