-
Notifications
You must be signed in to change notification settings - Fork 58
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
FED-1720 Conditionally null-safe builder output (tests) #868
FED-1720 Conditionally null-safe builder output (tests) #868
Conversation
Security InsightsNo security relevant content was detected by automated scans. Action Items
Questions or Comments? Reach out on Slack: #support-infosec. |
…nally-null-safe-builder # Conflicts: # lib/src/builder/builder.dart # pubspec.yaml
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.
Just one comment, otherwise copied over tests look good!
..._builder_integration_tests/new_boilerplate/accessor_mixin_integration_test.over_react.g.dart
Outdated
Show resolved
Hide resolved
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.
Just one small comment about an additional test coverage, otherwise this looks perfect, and I'll be ready to QA after that's addressed
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.
- Changes look good
- All tests under
non_null_safe_builder_integration_tests
look good, verified via:git diff \ origin/master:test/over_react/component_declaration/builder_integration_tests \ origin/FED-1720-conditionally-null-safe-builder:test/over_react/component_declaration/non_null_safe_builder_integration_tests
- All tests under
- Newly-added tests run and pass in CI
+10
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.
+1 in spirit
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.
+1
Motivation
Right now in the null safety branch, we have some logic in place around conditionally generating null-safe code, in order to support consumers migrating to null safety.
We currently support language version comments in files, but we don't have a mechanism for detecting package-level language versions.
Changes
pub run build_runner build
to inspect output of generated code, looking for comment that says how nullability was calculatedRelease Notes
Review
See CONTRIBUTING.md for more details on review types (+1 / QA +1 / +10) and code review process.
Please review:
QA Checklist
Merge Checklist
While we perform many automated checks before auto-merging, some manual checks are needed: