8000
  • fix: add oneof fields to generated protoplus init by crwilcox · Pull Request #485 · googleapis/gapic-generator-python · GitHub
    [go: up one dir, main page]
    Skip to content

    fix: add oneof fields to generated protoplus init#485

    Merged
    software-dov merged 32 commits intomasterfrom
    oneof-proto-templates
    Jul 7, 2020
    Merged

    fix: add oneof fields to generated protoplus init#485
    software-dov merged 32 commits intomasterfrom
    oneof-proto-templates

    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
    crwilcox commented Jun 26, 2020

    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
    8000 BenRKarl commented Jun 26, 2020

    @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
    crwilcox commented Jun 26, 2020

    @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
    0