-
Notifications
You must be signed in to change notification settings - Fork 9.8k
add a --dart-sdk option to the repo analysis command #3959
add a --dart-sdk option to the repo analysis command #3959
Conversation
Actually, I think that when I use this flag from the bash script:
./script/tool_runner.sh does not substitute the automatic list of CUSTOM_ANALYSIS_PLUGINS. I think we should remove that option, or, push the special casing for that from the bash script into the dart code. Otherwise I'll see:
|
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.
./script/tool_runner.sh does not substitute the automatic list of CUSTOM_ANALYSIS_PLUGINS. I think we should remove that option, or, push the special casing for that from the bash script into the dart code.
I'd like to remove it, but there are thousands of fixes to make first.
In the short term you can just fix lines 62 and 63 to check the first item in the list (62), and concatenate instead of replacing (63).
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.
Thanks for the review! Updates in-line.
In the short term you can just fix lines 62 and 63 to check the first item in the list (62), and concatenate instead of replacing (63).
Yup, I changed the check to compare the first arg w/ 'analysis', and to append the changes to the command line to the existing ACTIONS
variable.
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.
Mostly looks good, but you have a test failure in the tool tests; probably there's a mock that needs to be updated for the changes in how the analyze call is being made.
You'll also want to resync with the current master since the tree is finally green after several days of mostly red. That will resolve most of the CI failures, and let us see if there's anything other than the above that's real.
Ah, ok, I hadn't seen those tests. That test failure did catch a type error in my code. That's fixed, and I added a test for the new
Sync'd! I'll see if this builds through now. |
This pull request is not suitable for automatic merging in its current state.
|
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!
This PR adds an option to the 'flutter_plugin_tools analyze' command which allows you to replace the Dart SDK used to analyze the code. This will enable a Dart SDK built from head to analyze this repo; which will enable upstream testing of this repo before changes are committed to Dart's static analysis. Here's the delta to the current UI:
List which issues are fixed by this PR. You must list at least one issue.
If you had to change anything in the flutter/tests repo, include a link to the migration guide as per the breaking change policy.
Pre-launch Checklist
dart format
. See plugin_tool format)[shared_preferences]
///
).If you need help, consider asking for advice on the #hackers-new channel on Discord.