Skip to content

Add support for optional proto3 fields #157

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

Merged
merged 6 commits into from
Nov 22, 2020

Conversation

henribru
Copy link
Contributor

This adds support for optional proto3 fields, introduced as an experimental feature in 3.12. The only really noticeable difference in the API is that since they track field presence they can be used in HasFields, so besides declaring support for the feature the only change is allowing fields with proto3_optional set to True in HasFields.

Closes #146

See https://github.com/protocolbuffers/protobuf/blob/v3.12.0/docs/field_presence.md and https://github.com/protocolbuffers/protobuf/blob/v3.12.0/docs/implementing_proto3_presence.md for more info.

Note that some changes were required for the protobuf types in typeshed: python/typeshed#4680
While it's merged, it hasn't made its way into the stubs bundled with mypy yet.

@nipunn1313
Copy link
Owner

Hi! This looks great. Thank you for writing this up!

It appears to fail the mypy build because typeshed has some very old plugin.proto autogenerated pyi definitions.
I filed #158 to handle that.

For now, feel free to do a type:ignore attr-defined to bypass mypy check on the specific lines where you are seeing errors (w/ a comment referencing #158 ). I'll be happy to merge it once travis is passing!

https://mypy.readthedocs.io/en/stable/common_issues.html#:~:text=You%20can%20use%20the%20form,purpose%20of%20the%20comment%20is.

Also, if you have time, it'd be appreciated if you could add. If not, no worry, I can do it later.
1 - an entry to CHANGELOG.md under upcoming
2 - add yourself to the contributors list!

@henribru henribru force-pushed the proto3-optional-fields-support branch from 553b8c3 to fda51d9 Compare November 16, 2020 20:50
@nipunn1313
Copy link
Owner

Hi - tests are still not passing here. You can run the tests locally with ./run_test.sh. You can do expedited reruns locally with SKIP_CLEAN=1 ./run_test.sh - the test runner will automatically recreate the .expected files and negative files too.

The local test runner also runs mypy - so it should validate that things are working.

@nipunn1313
Copy link
Owner

looks like the expected negative results have updated - so you can take a look at those by running ./run_test.sh locally - or by looking at the results in CI

@nipunn1313
Copy link
Owner

From my quick readthrough of the errors - the "expected negative result" test files have updated, because the expected error message from mypy has changed - since for some cases the error message says something of the form "Expected a Union[a,b,c]" and w/ your change, that union has more members.

I think if you run ./run_test.sh locally, it'll update that file and then pass tests - so if you do that and update your diff w/ the new negative expected files, things will pass!

@henribru
Copy link
Contributor Author

henribru commented Nov 22, 2020

Unfortunately I've been having some trouble running the tests locally, I get errors like generated/google/protobuf/unittest_lazy_dependencies_pb2.pyi:12: error: Cannot find implementation or library stub for module named 'google.protobuf.unittest_lazy_dependencies_custom_option_pb2' [import] and then, as a consequence of those, errors like generated/google/protobuf/unittest_lazy_dependencies_pb2.pyi:37: error: Return type becomes "Any" due to an unfollowed import [no-any-unimported].

I'll try copying over the output from the diff on Travis to see if I can get the tests to pass there

@nipunn1313
Copy link
Owner

Ah that's unfortunate. I'd like to make the dev environment work nicely for everyone - so if you're willing, I'd like to try and get it working for you. If not, no worry - I can pick up the diff from you and complete it.

I recognize some of those errors. Can you try these steps one by one and see and let me know which one (if any) fixes your dev environment issue. If we can isolate the issue, we can put some kind of check in to make sure the environment is set up consistently.

  1. git clean -fdx from the root directory and try running again
  2. rebase on master and try again (particularly I'm hoping rebasing on top of 26ab89b will resolve for you)

If nothing works, can you paste the entire output of the run_test.sh command - and let me know if you'd like me to take over the diff.

This diff is great, and I'm sorry you're running into issues getting the tests to pass. I've tried to make the environment consistent (run mypy on a pinned version in a virtual env, confirm version of protoc, etc), but it's a tough task, so want to fix things as they come up. I really hope we can get it up and running for you.

@nipunn1313
Copy link
Owner

Looks like your strategy of copying the output on travis worked - so I'll merge it!

I'm still interested in helping debug your developer environment so we can avoid others running into the issue you had (if you have time). I'd also totally understand if not.

Thank you for the contribution!

@nipunn1313 nipunn1313 merged commit b24a447 into nipunn1313:master Nov 22, 2020
@henribru
Copy link
Contributor Author

git clean -fdx did the trick! 26ab89b was already present in my branch, but I probably ran the tests at some point before pulling in that change, since I had the compiled versions of those in my working tree

@henribru henribru deleted the proto3-optional-fields-support branch November 22, 2020 21:32
@nipunn1313
Copy link
Owner

Nice - I bet we can solve this issue for everyone by cleaning the generated/ directory every time - that directory isn't compiled incrementally, so it's at no cost to just do that, and should avoid this class of issues!

nipunn1313 added a commit that referenced this pull request Nov 22, 2020
This fixes the dev environment issue @henribru ran into in #157
I was able to repro the issue by building once on an older rev
and again on a newer rev.
nipunn1313 added a commit that referenced this pull request Nov 22, 2020
This fixes the dev environment issue @henribru ran into in #157
I was able to repro the issue by building once on an older rev
and again on a newer rev.
nipunn1313 added a commit that referenced this pull request Nov 23, 2020
This fixes the dev environment issue @henribru ran into in #157
I was able to repro the issue by building once on an older rev
and again on a newer rev.
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.

Support for optional proto3 fields
2 participants