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

SDK-780: Add regenerated protobuf files, remove .proto definitions #45

Merged
merged 2 commits into from Mar 25, 2019

Conversation

echarrod
Copy link
Contributor

Regenerating the protobuf files also generates gRPC files for each .proto definition.

I was tempted to remove these, as we haven't used them previously (and I am still optn to this), but this would mean we'd be interfering with the protobuf generation step, and would also need to do this every time we regenerate the Python files.

This issue was created which references the creation of these files was an error, but they decided in there it's best to leave the files as is for them as It's somewhat analogous to the API design decision of "if you write a function that returns a list of results, and a particular call of the function has no results, return an empty list rather than returning null and creating special cases for your users ".

Google seem to leave it in also- https://github.com/googleads/google-ads-python/blob/master/google/ads/google_ads/v1/proto/errors/extension_setting_error_pb2_grpc.py

Thoughts welcome!

Additionally, similar to in Ruby, the __init__.py file in attribute_public_api needed a modification to the sys.path, so that the classes are aware of each other. The alternative is using relative imports, but that would mean again modifying the generated files again.

There is talk of including this as a command line option in the Python protobuf generator (protocolbuffers/protobuf#1491), but for now this seems to be the only option which won't require modifying generated files or adding an extra step in the generation process.

Copy link
Contributor

@davidgrayston davidgrayston left a comment

Choose a reason for hiding this comment

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

  • Ran tests
  • Checked share worked using Flask example

Copy link

@bucky-boy bucky-boy left a comment

Choose a reason for hiding this comment

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

Looks good to me.

Leave the gRPC files if you want. They don't appear to do any harm, and you're right for saying it's we shouldn't have any manual changes in generated files. Key point is that SDK consumers should not be aware whether they are there or not, so we can remove them later if we see a need to and nobody will be any the wiser.

@echarrod echarrod merged commit f7b0aa8 into development Mar 25, 2019
@echarrod echarrod deleted the SDK-780-Protobuf branch March 25, 2019 10:18
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants