Skip to content
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

At least one user was mislead by the "--apply-changes" flag #40881

Closed
MichaelRFairhurst opened this issue Mar 4, 2020 · 9 comments
Closed

At least one user was mislead by the "--apply-changes" flag #40881

MichaelRFairhurst opened this issue Mar 4, 2020 · 9 comments
Assignees
Labels
area-migration (deprecated) Deprecated: this label is no longer actively used (was: issues with the `dart migrate` tool).
Milestone

Comments

@MichaelRFairhurst
Copy link
Contributor

MichaelRFairhurst commented Mar 4, 2020

cc @devoncarew

Talking to one of our early users, they ran dartdev migrate on a test package. After running the tool, they did not realize that there was a preview or that the preview was important. Instead they saw the advice "rerun with --apply-changes" and did that instead, and missed out on the main benefits of the tool.

We should rewrite the output to point users to the web preview more heavily, like even maybe something as extreme as:

*beep boop beep*: If you are a computer in an automated workflow, you can rerun
with --apply-changes. If you are a human, use the interactive web view to review,
improve, or apply the results:

Open URL http://.....:82340/....
@MichaelRFairhurst MichaelRFairhurst added area-analyzer Use area-analyzer for Dart analyzer issues, including the analysis server and code completion. analyzer-nnbd-migration labels Mar 4, 2020
@MichaelRFairhurst MichaelRFairhurst changed the title At least one user is mislead by the "--apply-changes" flag At least one user was mislead by the "--apply-changes" flag Mar 4, 2020
@devoncarew
Copy link
Member

It may be enough to print the preview page url in an ansi bold style?

@devoncarew
Copy link
Member

Playing with the verbiage sounds good also (use the interactive web view to review, improve, or apply the results), you just may want to steer away from specific computer / human guidance.

@jcollins-g
Copy link
Contributor

We can also check if the output is a terminal, and if so directly pop up a web browser tab pointing to the right URI. While this would have to be implemented at least 3 different ways depending on platform, it would help direct people into the UI and it would cut down on the annoyance factor of cutting and pasting URIs.

@devoncarew
Copy link
Member

There's an existing pub package for this (https://pub.dev/packages/browser_launcher), maintained by the Dart team (for use by DevTools, among other tools).

@devoncarew
Copy link
Member

Some work for this is here: https://dart-review.googlesource.com/c/sdk/+/138480 (modulo launching in a browser).

dart-bot pushed a commit that referenced this issue Mar 5, 2020
Bug: #40881
Change-Id: Ie5b63410a926462352858f8d4341e79a8c332be5
Reviewed-on: https://dart-review.googlesource.com/c/sdk/+/138480
Reviewed-by: Mike Fairhurst <mfairhurst@google.com>
Reviewed-by: Janice Collins <jcollins@google.com>
Commit-Queue: Devon Carew <devoncarew@google.com>
@stereotype441
Copy link
Member

This has been improved--the new behavior is that by default, the migration tool starts up the preview server. But if --no-web-preview is supplied, the tool still says To apply these changes, re-run the tool with --apply-changes. I still think we should consider improving the verbiage, e.g. as suggested in #40881 (comment).

@stereotype441
Copy link
Member

Mike also points out that when --no-web-preview is not supplied, the tool still tells you about --apply-changes. Probably we shouldn't explicitly point the user to this option since we want to discourage its use. They can find it in --help if they really need it.

@franklinyow franklinyow added this to the D29 Release milestone Apr 8, 2020
@srawlins
Copy link
Member

srawlins commented May 6, 2020

This issue should be defined more concretely. It looks like the AIs are:

  • Launch the browser (and add a flag, --[no]launch-browser?).

  • If the web preview is served, do not print text about --apply-changes.

  • The current text is awkward:

    View migration results:
    Visit:
    
      http://localhost:54770/Users/srawlins/code/dart-csslib?authToken=...
    
    to see the migration results. Use the interactive web view to review...
    

    I like the word "suggestions" used earlier in the output. Maybe something like:

    View the migration suggestions by visiting:
    
      http://localhost:54770/Users/srawlins/code/dart-csslib?authToken=...
    
    Use this interactive preview to review...
    

@srawlins srawlins self-assigned this May 7, 2020
@srawlins
Copy link
Member

srawlins commented May 7, 2020

I've forked off the "launch a browser" idea into its own issue: #41809.

@stereotype441 stereotype441 added area-migration (deprecated) Deprecated: this label is no longer actively used (was: issues with the `dart migrate` tool). and removed analyzer-nnbd-migration area-analyzer Use area-analyzer for Dart analyzer issues, including the analysis server and code completion. labels Nov 16, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
area-migration (deprecated) Deprecated: this label is no longer actively used (was: issues with the `dart migrate` tool).
Projects
None yet
Development

No branches or pull requests

6 participants