Skip to content
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: Add typing to proto.Message based class attributes #1474

Merged
merged 10 commits into from Nov 9, 2022
Merged

feat: Add typing to proto.Message based class attributes #1474

merged 10 commits into from Nov 9, 2022

Conversation

PicardParis
Copy link
Contributor

Fixes #1379

@product-auto-label product-auto-label bot added the size: l Pull request size is large. label Oct 18, 2022
@PicardParis PicardParis marked this pull request as ready for review October 18, 2022 14:37
@PicardParis PicardParis requested review from a team as code owners October 18, 2022 14:37
@parthea parthea added the owlbot:run Add this label to trigger the Owlbot post processor. label Oct 18, 2022
@gcf-owl-bot gcf-owl-bot bot removed the owlbot:run Add this label to trigger the Owlbot post processor. label Oct 18, 2022
@parthea parthea added the owlbot:run Add this label to trigger the Owlbot post processor. label Oct 19, 2022
@gcf-owl-bot gcf-owl-bot bot removed the owlbot:run Add this label to trigger the Owlbot post processor. label Oct 19, 2022
@parthea parthea added the owlbot:run Add this label to trigger the Owlbot post processor. label Oct 26, 2022
@gcf-owl-bot gcf-owl-bot bot removed the owlbot:run Add this label to trigger the Owlbot post processor. label Oct 26, 2022
@product-auto-label product-auto-label bot added size: xl Pull request size is extra large. and removed size: l Pull request size is large. labels Oct 27, 2022
@PicardParis
Copy link
Contributor Author

Here is the rationale for the latest changes:

  1. proto.RepeatedField
    • The runtime type is proto.marshal.collections.repeated.RepeatedComposite
    • RepeatedComposite is a subclass of MutableSequence:
      • class Repeated(collections.abc.MutableSequence): ...
      • class RepeatedComposite(Repeated): ...
    • See implementation
  2. proto.MapField
    • The runtime type is proto.marshal.collections.maps.MapComposite
    • MapComposite is a subclass of MutableMapping:
      • class MapComposite(collections.abc.MutableMapping): ...
    • See implementation
  3. Parts of the code do use these attributes as mutable:
  4. Doc annotations incorrectly indicated Sequence & Mapping
  5. Goal of the changes: SequenceMutableSequence & MappingMutableMapping

@parthea parthea added the owlbot:run Add this label to trigger the Owlbot post processor. label Oct 28, 2022
@gcf-owl-bot gcf-owl-bot bot removed the owlbot:run Add this label to trigger the Owlbot post processor. label Oct 28, 2022
@parthea
Copy link
Contributor

parthea commented Oct 28, 2022

I'm going to close and re-open this PR to re-trigger gh actions.

@parthea parthea closed this Oct 28, 2022
@parthea parthea reopened this Oct 28, 2022
@product-auto-label product-auto-label bot added size: l Pull request size is large. and removed size: xl Pull request size is extra large. labels Oct 28, 2022
@product-auto-label product-auto-label bot added size: xl Pull request size is extra large. and removed size: l Pull request size is large. labels Nov 8, 2022
@PicardParis
Copy link
Contributor Author

...fixing conflicts due to updates in the main branch...

@PicardParis PicardParis marked this pull request as draft November 8, 2022 16:45
@product-auto-label product-auto-label bot added size: l Pull request size is large. and removed size: xl Pull request size is extra large. labels Nov 8, 2022
@PicardParis PicardParis marked this pull request as ready for review November 8, 2022 18:58
@parthea parthea added the owlbot:run Add this label to trigger the Owlbot post processor. label Nov 8, 2022
@gcf-owl-bot gcf-owl-bot bot removed the owlbot:run Add this label to trigger the Owlbot post processor. label Nov 8, 2022
Copy link
Contributor

@parthea parthea left a comment

Choose a reason for hiding this comment

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

I generated the code locally for datastore and the mypy test failed with these changes.
See generated code here: https://github.com/googleapis/python-datastore/compare/test-local-gapic?expand=1

@PicardParis
Copy link
Contributor Author

  • I made another pass. Tests are successful.
  • I was not able to reproduce your issue when generating datastore locally.
    • We must be using different generation commands.
    • I tried
      protoc google/datastore/v1/*.proto \
        --proto_path=../api-common-protos/ --proto_path=. \
        --python_gapic_out=../dest/
      
      and
      protoc google/datastore/admin/v1/*.proto \
        --proto_path=../api-common-protos/ --proto_path=. \
        --python_gapic_out=../dest/
      
      and nox -s mypy completes for both.
    • Which command do you use to generate datastore?
  • Anyway, the new pass should be exhaustive. Can you recheck?

@parthea parthea added the owlbot:run Add this label to trigger the Owlbot post processor. label Nov 9, 2022
@gcf-owl-bot gcf-owl-bot bot removed the owlbot:run Add this label to trigger the Owlbot post processor. label Nov 9, 2022
Copy link
Contributor

@parthea parthea left a comment

Choose a reason for hiding this comment

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

LGTM

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
size: l Pull request size is large.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Fix attribute types for autogenerated protobuf classes
2 participants