[flutter_tools] Clean up `boolArgDeprecated` and `stringArgDeprecated` by bartekpacia · Pull Request #122184 · flutter/flutter

bartekpacia · 2023-03-08T14:16:31Z

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.

bartekpacia · 2023-03-08T17:03:01Z

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 · 2023-03-08T17:11:45Z

Oh, I think I found a fix :)

packages/flutter_tools/lib/src/runner/flutter_command.dart

packages/flutter_tools/lib/src/commands/analyze.dart

packages/flutter_tools/lib/src/commands/attach.dart

packages/flutter_tools/lib/src/commands/drive.dart

packages/flutter_tools/lib/src/runner/flutter_command.dart

packages/flutter_tools/test/commands.shard/hermetic/run_test.dart

bartekpacia · 2023-03-08T21:37:21Z

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

christopherfujino

LGTM

andrewkolos

LGTM with nit

packages/flutter_tools/lib/src/commands/analyze.dart

auto-submit · 2023-03-13T17:01:20Z

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 · 2023-03-13T17:03:04Z

Looks like you have a conflict with #121903

Co-authored-by: Christopher Fujino <[email protected]>

bartekpacia · 2023-03-13T17:04:22Z

Yup, thanks for heads up, fixed :)

bartekpacia · 2023-03-13T18:29:06Z

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

Oopsie, @christopherfujino?

auto-submit · 2023-03-13T18:32:50Z

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.

…precated` (flutter/flutter#122184)

flutter-dashboard bot added the tool Affects the "flutter" command-line tool. See also t: labels. label Mar 8, 2023

bartekpacia marked this pull request as ready for review March 8, 2023 17:12

christopherfujino reviewed Mar 8, 2023

View reviewed changes

packages/flutter_tools/lib/src/runner/flutter_command.dart Outdated Show resolved Hide resolved

christopherfujino reviewed Mar 8, 2023

View reviewed changes

bartekpacia requested a review from christopherfujino March 8, 2023 21:09

christopherfujino approved these changes Mar 9, 2023

View reviewed changes

christopherfujino requested a review from andrewkolos March 9, 2023 17:55

andrewkolos approved these changes Mar 12, 2023

View reviewed changes

packages/flutter_tools/lib/src/commands/analyze.dart Outdated Show resolved Hide resolved

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 bot removed the autosubmit Merge PR when tree becomes green via auto submit App label Mar 13, 2023

change comment of boolArgDeprecated, which is no longer deprecated

a1e7e75

bartekpacia and others added 13 commits March 13, 2023 18:03

delete boolArg, rename boolArgDeprecated to boolArg

7675d52

fix unnecessary usages of ?? on boolArg

25f22b3

delete stringArg, rename stringArgDeprecated to stringArg

f92d211

fix failing tests (throw TypeError if argResults is null)

8389b4c

make non-existent option more obvious in args_test.dart

d411d06

fix test failures

b5ff485

Update packages/flutter_tools/lib/src/runner/flutter_command.dart

28373ff

Co-authored-by: Christopher Fujino <[email protected]>

Update packages/flutter_tools/lib/src/commands/analyze.dart

c7e2545

Co-authored-by: Christopher Fujino <[email protected]>

Improve doc comment for FlutterCommand.boolArg

52cc450

Co-authored-by: Christopher Fujino <[email protected]>

Improve doc comment for FlutterCommand.stringArg

6150281

Co-authored-by: Christopher Fujino <[email protected]>

AttachCommand.debugUri: simplify code

b8367ae

fix test

6eca189

fix analyzer warning in BuildApkCommand

2a02001

analyze.dart: remove redundant finals

019cd18

christopherfujino added the autosubmit Merge PR when tree becomes green via auto submit App label Mar 13, 2023

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

christopherfujino added the autosubmit Merge PR when tree becomes green via auto submit App label Mar 13, 2023

auto-submit bot merged commit 37fc9ed into flutter:master Mar 13, 2023

bartekpacia deleted the refactor/cleanup_cli_arg_accessors branch March 13, 2023 21:20

engine-flutter-autoroll mentioned this pull request Mar 14, 2023

Roll Flutter from 9ba0d08ebc07 to ac37dd261110 (23 revisions) flutter/packages#3456

Closed

engine-flutter-autoroll added a commit to engine-flutter-autoroll/packages that referenced this pull request Mar 14, 2023

37fc9ed [flutter_tools] Clean up boolArgDeprecated and `stringArgDe…

d89fea5

…precated` (flutter/flutter#122184)

engine-flutter-autoroll mentioned this pull request Mar 14, 2023

Roll Flutter from 9ba0d08ebc07 to d8a721106108 (24 revisions) flutter/packages#3457

Merged

engine-flutter-autoroll added a commit to engine-flutter-autoroll/packages that referenced this pull request Mar 14, 2023

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

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

37fc9ed [flutter_tools] Clean up boolArgDeprecated and `stringArgDe…

ff28351

…precated` (flutter/flutter#122184)

Conversation

bartekpacia commented Mar 8, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Pre-launch Checklist

Uh oh!

bartekpacia commented Mar 8, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

bartekpacia commented Mar 8, 2023

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

bartekpacia commented Mar 8, 2023

Uh oh!

christopherfujino left a comment

Choose a reason for hiding this comment

Uh oh!

andrewkolos left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

auto-submit bot commented Mar 13, 2023

Uh oh!

christopherfujino commented Mar 13, 2023

Uh oh!

bartekpacia commented Mar 13, 2023

Uh oh!

bartekpacia commented Mar 13, 2023

Uh oh!

auto-submit bot commented Mar 13, 2023

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

bartekpacia commented Mar 8, 2023 •

edited

Loading

bartekpacia commented Mar 8, 2023 •

edited

Loading