Skip to content

[flutter_tools] Improve toolexit error messages for flutter upgrade - #86351

Merged
christopherfujino merged 4 commits into
flutter:masterfrom
royarg02:update_fail_msg
Jul 30, 2021
Merged

[flutter_tools] Improve toolexit error messages for flutter upgrade#86351
christopherfujino merged 4 commits into
flutter:masterfrom
royarg02:update_fail_msg

Conversation

@royarg02

@royarg02 royarg02 commented Jul 13, 2021

Copy link
Copy Markdown
Contributor

This PR updates the toolexit error messages displayed when one runs flutter upgrade with either the HEAD not pointing to a branch, or the current branch lacking an upstream.

More specifically, this removes any git commands from the error messages. and replaces them with an equivalent from the flutter sub commands, if any.

Also adds a link to the official Flutter install docs(https://flutter.dev/docs/get-started/install) for further help.

This change was originally a part of #79372, which was reverted in #83423.

Fixes #49713.
Fixes #79366.

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 feature I am adding, or Hixie said the 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 Jul 13, 2021
@google-cla google-cla Bot added the cla: yes label Jul 13, 2021
christopherfujino
christopherfujino suggested changes Jul 16, 2021
View reviewed changes
Comment thread packages/flutter_tools/test/commands.shard/permeable/upgrade_test.dart Outdated

@christopherfujino christopherfujino Jul 16, 2021

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Any reason not to use expectLater() with throwsToolExit()?

@royarg02 royarg02 Jul 16, 2021

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

I need to test for multiple strings within the error message. @jonahwilliams suggested over here the way to do this, but I'm all for any other approach.

@christopherfujino christopherfujino Jul 16, 2021

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Can you not match the entire message as a single string?

@royarg02 royarg02 Jul 17, 2021

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

Wouldn't that break these tests over trivial formatting changes?

@royarg02 royarg02 Jul 17, 2021

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

I guess breaking tests would be worth the tradeoff for that kind of changes. Nvm.

Comment thread packages/flutter_tools/lib/src/commands/upgrade.dart Outdated

@christopherfujino christopherfujino Jul 16, 2021

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

nit make this a const String

@royarg02 royarg02 Jul 16, 2021

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

Done.

@royarg02
royarg02 force-pushed the update_fail_msg branch 5 times, most recently from 3532511 to a858887 Compare July 17, 2021 05:50
jonahwilliams
jonahwilliams reviewed Jul 22, 2021
View reviewed changes
Comment thread packages/flutter_tools/lib/src/commands/upgrade.dart Outdated
Comment on lines 251 to 252

@jonahwilliams jonahwilliams Jul 22, 2021

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

The first message is better. I agree we should point users away from using git directly, but then terminology like Flutter: HEAD is just going to be confusing.

@royarg02 royarg02 Jul 23, 2021

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

How about this?

Unable to upgrade Flutter: Your Flutter checkout is currently not on a release branch (Are you
in a detached HEAD state?)...

I would prefer to keep the comment within () to suggest users who could diagnose this error with git.

@christopherfujino christopherfujino Jul 23, 2021

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

i think if they know what they're doing, "your flutter checkout is currently not on a release branch" will let them know what's happening. and if not, they can always just run the flutter channel command.

@royarg02 royarg02 Jul 23, 2021

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

Sounds reasonable. Likewise, I could edit the other message:

Unable to upgrade Flutter: The current Flutter branch/channel is not tracking any remote repository...

If it is okay to lose some verbosity in these messages, I will go ahead and make the changes.

@christopherfujino christopherfujino Jul 23, 2021

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

That looks good to me.

@royarg02

royarg02 commented Jul 30, 2021

Copy link
Copy Markdown
Contributor Author

@jonahwilliams @christopherfujino friendly ping

christopherfujino
christopherfujino approved these changes Jul 30, 2021
View reviewed changes

@christopherfujino christopherfujino left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

LGTM

@christopherfujino
christopherfujino merged commit ade9f6a into flutter:master Jul 30, 2021
@royarg02
royarg02 deleted the update_fail_msg branch December 18, 2021 13:01
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

2 more reviewers
@jonahwilliams jonahwilliams jonahwilliams left review comments
@christopherfujino christopherfujino christopherfujino approved these changes
Reviewers whose approvals may not affect merge requirements

Assignees

No one assigned

Labels

tool Affects the "flutter" command-line tool. See also t: labels.

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

[flutter_tools] flutter upgrade gives incomplete instructions to fix missing upstream remote flutter upgrade: unable to recognized remote origin

3 participants

@royarg02 @christopherfujino @jonahwilliams