-
Notifications
You must be signed in to change notification settings - Fork 3k
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
[pigeon] adds option for java output to have the Generated annotation #2261
Conversation
LGTM. But I can't find the approval button on my UI. I may not have enough permission. |
/// Determines if the `javax.annotation.Generated` is used in the output. This | ||
/// is false by default since that dependency isn't available in plugins by | ||
/// default . | ||
final bool? useGeneratedAnnotation; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Why not non-nullable and defaulting to false? It's not clear to me what null
would mean that would be different from false
.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Null means the option hasn't been specified, thus use the default value. It was originally implemented as nonnull but it resulted in duplication of the default value. By keeping it undefined until it's actually used I have a single point of truth about what the default value is.
test('parse args - java_use_generated_annotation', () { | ||
final PigeonOptions opts = | ||
Pigeon.parseArgs(<String>['--java_use_generated_annotation']); | ||
expect(opts.javaOptions!.useGeneratedAnnotation, isTrue); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
There should be a test that the default is actually false as well.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Done
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM
fixes flutter/flutter#105653
Pre-launch Checklist
dart format
.)[shared_preferences]
pubspec.yaml
with an appropriate new version according to the pub versioning philosophy, or this PR is exempt from version changes.CHANGELOG.md
to add a description of the change, following repository CHANGELOG style.///
).If you need help, consider asking for advice on the #hackers-new channel on Discord.