Skip to content

[flutter_tools] Clean up boolArgDeprecated and stringArgDeprecated #122184

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.

Sign up for GitHub

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

Jump to bottom
Merged
auto-submit merged 15 commits into from
Mar 13, 2023
Merged

[flutter_tools] Clean up boolArgDeprecated and stringArgDeprecated #122184

auto-submit merged 15 commits into from
Mar 13, 2023

Conversation

@bartekpacia
Copy link
Member

@bartekpacia bartekpacia commented Mar 8, 2023 •
edited
Loading

This PR standarizes the methods used to parse command-line flags and options.

Fixes #105263 as discussed in #105263 (comment).

Pre-launch Checklist

  • I read the Contributor Guide and followed the process outlined there for submitting PRs.
  • I read the Tree Hygiene wiki page, which explains my responsibilities.
  • I read and followed the Flutter Style Guide, including Features we expect every widget to implement.
  • I signed the CLA.
  • I listed at least one issue that this PR fixes in the description above.
  • I updated/added relevant documentation (doc comments with ///).
  • I added new tests to check the change I am making, or this PR is test-exempt.
  • All existing and new tests are passing.

@flutter-dashboard flutter-dashboard bot added the tool Affects the "flutter" command-line tool. See also t: labels. label Mar 8, 2023
@bartekpacia
Copy link
Member Author

bartekpacia commented Mar 8, 2023 •
edited
Loading

The 3 test failures (Mac tool_tests_commands, Windows tool_tests_commands, tool_tests-commands-linux) are coming from the single group in test/commands.shard/hermetic/run_test.dart – usageValues. I think there's a bug in that test that became visible just now that boolArg throws if argResults is null (instead of falling back to false, as it did before).

It crashes in these lines: 676, 699, and a couple of the same ones in usageValues group.

Crashes in these lines come from here in RunCommand.usageValues. The tests that fail don't call run() so argResults are null, and hence the crash.

@christopherfujino @jmagman

@bartekpacia
Copy link
Member Author

bartekpacia commented Mar 8, 2023

Oh, I think I found a fix :)

@bartekpacia bartekpacia marked this pull request as ready for review March 8, 2023 17:12
christopherfujino
christopherfujino reviewed Mar 8, 2023
View reviewed changes
christopherfujino
christopherfujino reviewed Mar 8, 2023
View reviewed changes
@bartekpacia
Copy link
Member Author

bartekpacia commented Mar 8, 2023

Hmm, looks like the Mac module_custom_host_app_name_test has timed out. Don't know why's that.

christopherfujino
christopherfujino approved these changes Mar 9, 2023
View reviewed changes
Copy link
Contributor

@christopherfujino christopherfujino left a comment

Choose a reason for hiding this comment

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

LGTM

andrewkolos
andrewkolos approved these changes Mar 12, 2023
View reviewed changes
Copy link
Contributor

@andrewkolos andrewkolos left a comment

Choose a reason for hiding this comment

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

LGTM with nit

@christopherfujino christopherfujino added autosubmit Merge PR when tree becomes green via auto submit App and removed tool Affects the "flutter" command-line tool. See also t: labels. labels Mar 13, 2023
@auto-submit auto-submit bot removed the autosubmit Merge PR when tree becomes green via auto submit App label Mar 13, 2023
@auto-submit
Copy link
Contributor

auto-submit bot commented Mar 13, 2023

auto label is removed for flutter/flutter, pr: 122184, due to - This commit is not mergeable and has conflicts. Please rebase your PR and fix all the conflicts.

@christopherfujino
Copy link
Contributor

christopherfujino commented Mar 13, 2023

Looks like you have a conflict with #121903

bartekpacia and others added 13 commits March 13, 2023 18:03
@bartekpacia
delete boolArg, rename boolArgDeprecated to boolArg
7675d52
@bartekpacia
fix unnecessary usages of ?? on boolArg
25f22b3
@bartekpacia
delete stringArg, rename stringArgDeprecated to stringArg
f92d211
@bartekpacia
fix failing tests (throw TypeError if argResults is null)
8389b4c
@bartekpacia
make non-existent option more obvious in args_test.dart
d411d06
@bartekpacia
fix test failures
b5ff485
@bartekpacia @christopherfujino
Update packages/flutter_tools/lib/src/runner/flutter_command.dart
28373ff 
Co-authored-by: Christopher Fujino <[email protected]>
@bartekpacia @christopherfujino
Update packages/flutter_tools/lib/src/commands/analyze.dart
c7e2545 
Co-authored-by: Christopher Fujino <[email protected]>
@bartekpacia @christopherfujino
Improve doc comment for FlutterCommand.boolArg
52cc450 
Co-authored-by: Christopher Fujino <[email protected]>
@bartekpacia @christopherfujino
Improve doc comment for FlutterCommand.stringArg
6150281 
Co-authored-by: Christopher Fujino <[email protected]>
@bartekpacia
AttachCommand.debugUri: simplify code
b8367ae
@christopherfujino @bartekpacia
fix test
6eca189
@bartekpacia
fix analyzer warning in BuildApkCommand
2a02001
@bartekpacia
Copy link
Member Author

bartekpacia commented Mar 13, 2023

Yup, thanks for heads up, fixed :)

@christopherfujino christopherfujino added the autosubmit Merge PR when tree becomes green via auto submit App label Mar 13, 2023
@bartekpacia
Copy link
Member Author

bartekpacia commented Mar 13, 2023

Google testing failed. Please contact a Google developer to investigate.

Oopsie, @christopherfujino?

@auto-submit auto-submit bot removed the autosubmit Merge PR when tree becomes green via auto submit App label Mar 13, 2023
@auto-submit
Copy link
Contributor

auto-submit bot commented Mar 13, 2023

auto label is removed for flutter/flutter, pr: 122184, due to - The status or check suite Google testing has failed. Please fix the issues identified (or deflake) before re-applying this label.

@christopherfujino christopherfujino added the autosubmit Merge PR when tree becomes green via auto submit App label Mar 13, 2023
@auto-submit auto-submit bot merged commit 37fc9ed into flutter:master Mar 13, 2023
@bartekpacia bartekpacia deleted the refactor/cleanup_cli_arg_accessors branch March 13, 2023 21:20
@engine-flutter-autoroll engine-flutter-autoroll mentioned this pull request Mar 14, 2023
Closed
engine-flutter-autoroll added a commit to engine-flutter-autoroll/packages that referenced this pull request Mar 14, 2023
@engine-flutter-autoroll
37fc9ed [flutter_tools] Clean up boolArgDeprecated and `stringArgDe…
d89fea5 
…precated` (flutter/flutter#122184)
@engine-flutter-autoroll engine-flutter-autoroll mentioned this pull request Mar 14, 2023
Merged
engine-flutter-autoroll added a commit to engine-flutter-autoroll/packages that referenced this pull request Mar 14, 2023
@engine-flutter-autoroll
37fc9ed [flutter_tools] Clean up boolArgDeprecated and `stringArgDe…
1a358a9 
…precated` (flutter/flutter#122184)
engine-flutter-autoroll added a commit to engine-flutter-autoroll/packages that referenced this pull request May 10, 2023
@engine-flutter-autoroll
37fc9ed [flutter_tools] Clean up boolArgDeprecated and `stringArgDe…
ee750e0 
…precated` (flutter/flutter#122184)
engine-flutter-autoroll added a commit to engine-flutter-autoroll/packages that referenced this pull request May 10, 2023
@engine-flutter-autoroll
37fc9ed [flutter_tools] Clean up boolArgDeprecated and `stringArgDe…
ff28351 
…precated` (flutter/flutter#122184)
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@andrewkolos andrewkolos andrewkolos approved these changes

+1 more reviewer

@christopherfujino christopherfujino christopherfujino approved these changes

Reviewers whose approvals may not affect merge requirements

Assignees

No one assigned

Labels

autosubmit Merge PR when tree becomes green via auto submit App

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

[flutter_tools] boolArg() should be non-nullable, and throw if the flag doesn't exist

3 participants

@bartekpacia @christopherfujino @andrewkolos