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

fix: add oneof fields to generated protoplus init #485

Merged
merged 32 commits into from
Jul 7, 2020

Conversation

crwilcox
Copy link
Contributor

@crwilcox crwilcox commented Jun 25, 2020

Fixes: #484

@googlebot googlebot added the cla: yes This human has signed the Contributor License Agreement. label Jun 25, 2020
@crwilcox crwilcox added the do not merge Indicates a pull request not ready for merge, due to either quality or timing. label Jun 25, 2020
@crwilcox
Copy link
Contributor Author

It seems perhaps the data for oneofs may not be captured?

The template ought to be right provided oneof makes it to the field.

# TODO(lukesneeringer): oneofs are on path 7.

# self._load_children(message.oneof_decl, loader=self._load_field,

@crwilcox crwilcox requested a review from lukesneeringer June 26, 2020 05:23
@BenRKarl
Copy link
Contributor

@crwilcox is the issue here that oneof fields aren't present at all in your generated classes? If so I think we have the same problem for Ads, if a change is made here can we also change the ads templates? See here

@crwilcox
Copy link
Contributor Author

@BenRKarl For sure. Thanks for pointing that out. Still need to figure out piping the oneof data down to the field I think.

crwilcox added a commit to crwilcox/python-firestore that referenced this pull request Jun 26, 2020
@crwilcox crwilcox requested a review from software-dov June 30, 2020 00:05
@crwilcox crwilcox removed the do not merge Indicates a pull request not ready for merge, due to either quality or timing. label Jun 30, 2020
gapic/schema/api.py Outdated Show resolved Hide resolved
Copy link
Contributor

@software-dov software-dov 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. Can't wait to see the additional tests.

gapic/schema/api.py Outdated Show resolved Hide resolved
gapic/schema/api.py Outdated Show resolved Hide resolved
gapic/schema/api.py Outdated Show resolved Hide resolved
gapic/schema/api.py Show resolved Hide resolved
@codecov
Copy link

codecov bot commented Jul 7, 2020

Codecov Report

Merging #485 into master will not change coverage.
The diff coverage is 100.00%.

Impacted file tree graph

@@            Coverage Diff            @@
##            master      #485   +/-   ##
=========================================
  Coverage   100.00%   100.00%           
=========================================
  Files           26        26           
  Lines         1466      1487   +21     
  Branches       300       303    +3     
=========================================
+ Hits          1466      1487   +21     
Impacted Files Coverage Δ
gapic/schema/api.py 100.00% <100.00%> (ø)
gapic/schema/wrappers.py 100.00% <100.00%> (ø)
gapic/utils/__init__.py 100.00% <100.00%> (ø)
gapic/utils/code.py 100.00% <100.00%> (ø)

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 9076362...8dbaa75. Read the comment docs.

@crwilcox crwilcox added cla: yes This human has signed the Contributor License Agreement. and removed cla: yes This human has signed the Contributor License Agreement. labels Jul 7, 2020
@software-dov software-dov added cla: yes This human has signed the Contributor License Agreement. and removed cla: yes This human has signed the Contributor License Agreement. labels Jul 7, 2020
@software-dov software-dov merged commit be5a847 into master Jul 7, 2020
@software-dov software-dov deleted the oneof-proto-templates branch July 7, 2020 22:47
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
cla: yes This human has signed the Contributor License Agreement.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

oneof proto not generating as oneof
5 participants