-
Notifications
You must be signed in to change notification settings - Fork 23
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
Null safety 3 #192
Null safety 3 #192
Conversation
analysis_options.yaml
Outdated
@@ -8,7 +8,7 @@ analyzer: | |||
# When compiling to JS, both implicit options apply to the current | |||
# project and all dependencies. They are useful to find possible | |||
# Type fixes or areas for explicit typing. | |||
implicit-casts: true | |||
implicit-casts: 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.
I think we should use the replacements available since 2.16 https://dart.dev/tools/analysis#enabling-additional-type-checks
which would mean we'd also need to raise the SDK minimum to at least 2.16
analyzer:
language:
strict-casts: true
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.
Will this replace:
analyzer:
strong-mode:
implicit-casts: true
implicit-dynamic: true
or just implicit-casts: true
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.
It would replace the whole strong-mode block.
implicit-dynamic: true is the default and doesn't do anything, so can safely be removed.
pubspec.yaml
Outdated
@@ -19,3 +19,9 @@ dev_dependencies: | |||
dart_dev: ^4.0.0 | |||
dependency_validator: ^3.0.0 | |||
test: ^1.18.2 | |||
|
|||
dependency_overrides: |
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.
TODO Once react releases, we'll need to remove this override and set the react dependency minimum to the new react release (line 12 above)
test/action_test.dart
Outdated
@@ -387,14 +387,14 @@ void main() { | |||
test('should support dispatch with a null payload', () async { | |||
nullAction.listen(expectAsync1((payload) { | |||
expect(payload, isNull); | |||
})); | |||
} as dynamic Function(Null))); |
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.
#nit The migration tool seems to love to add the as casts ... is this needed or can it be fixed a different way? Since the tests pass, then I consider this optional.
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.
It looks like it is not needed. I removed the cast and the analyzer seems ok with it.
Security InsightsNo security relevant content was detected by automated scans. Action Items
Questions or Comments? Reach out on Slack: #support-infosec. |
example/analysis-options.yaml
Outdated
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.
This file name should be analysis_options.yaml
@Workiva/release-management-p ready to merge 🎉 |
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 from RM
Motivation
Now that we have implemented ActionV2 in w_flux 2.11.0 we can safely migrate this repo to null-safety
Changes
Run the dart migrate tool for the root app and the example app
Release 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: