Skip to content

fix: add oneof fields to generated protoplus init #485

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.

Sign up for GitHub

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

Jump to bottom
Merged
merged 32 commits into from
Jul 7, 2020
Merged

fix: add oneof fields to generated protoplus init #485

merged 32 commits into from
Jul 7, 2020

Conversation

crwilcox
Copy link
Contributor

@crwilcox crwilcox commented Jun 25, 2020 •
edited
Loading

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.

gapic-generator-python/gapic/schema/api.py

Line 471 in 920e419

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

gapic-generator-python/gapic/schema/api.py

Line 782 in 920e419

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

@crwilcox crwilcox requested a review from lukesneeringer June 26, 2020 05:23
@crwilcox crwilcox mentioned this pull request Jun 26, 2020
Closed
@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
Manual fixes for googleapis/gapic-generator-python#485
1b856e3
@busunkim96 busunkim96 mentioned this pull request Jun 29, 2020
Closed
crwilcox
crwilcox commented Jun 30, 2020
View reviewed changes
@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
lukesneeringer
lukesneeringer reviewed Jun 30, 2020
View reviewed changes
software-dov
software-dov reviewed Jun 30, 2020
View reviewed changes
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.

@codecov
Copy link

codecov bot commented Jul 7, 2020 •
edited
Loading

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
This was referenced Jan 18, 2022
Merged
Closed
This was referenced Jan 26, 2022
Closed
Closed
Merged
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

2 more reviewers

@lukesneeringer lukesneeringer lukesneeringer approved these changes

@software-dov software-dov software-dov approved these changes

Reviewers whose approvals may not affect merge requirements

Assignees

No one assigned

Labels

cla: yes This human has signed the Contributor License Agreement.

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

oneof proto not generating as oneof

5 participants

@crwilcox @BenRKarl @lukesneeringer @software-dov @googlebot