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

Remove Extension.repeated check argument #740

Open
wants to merge 1 commit into
base: master
Choose a base branch
from

Conversation

osa1
Copy link
Member

@osa1 osa1 commented Aug 24, 2022

This argument is always passed the same value in the generated code, so we can remove the function parameter and use the value passed by the generated code as the value of the parameter. This saves one line per extension field in the generated code.

@osa1 osa1 requested a review from sigurdm August 24, 2022 13:12
@sigurdm
Copy link
Collaborator

sigurdm commented Aug 25, 2022

The lack of test changes indicates missing coverage.

Copy link
Collaborator

@sigurdm sigurdm left a comment

Choose a reason for hiding this comment

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

LGTM

@osa1
Copy link
Member Author

osa1 commented Aug 25, 2022

The lack of test changes indicates missing coverage.

We have extension tests in protoc_plugin (extension_test.dart).

@sigurdm
Copy link
Collaborator

sigurdm commented Aug 25, 2022

But it seems we have no golden-file coverage of the generated code.... maybe that is for the better...

@osa1
Copy link
Member Author

osa1 commented Aug 25, 2022

Yeah we don't have golden files for extensions.

Tbh I'm not sure why we have golden file tests.. What are they supposed to be checking?

@sigurdm
Copy link
Collaborator

sigurdm commented Aug 26, 2022

They are a safety guard for changes in the output.

I guess with perfect functional test-coverage we could argue they would not be useful.
But I am always happy when reviewing that I can follow a change in the generator in the expected output.

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.

2 participants