Skip to content
This repository has been archived by the owner on Jun 1, 2023. It is now read-only.

Disambiguate -s as store option #1879

Merged
merged 2 commits into from
Dec 23, 2021
Merged

Disambiguate -s as store option #1879

merged 2 commits into from
Dec 23, 2021

Conversation

amcaplan
Copy link
Contributor

@amcaplan amcaplan commented Dec 22, 2021 •
edited
Loading

WHY are these changes introduced?

Better error experience when the user mistypes --store as -store. This happens at least 100 times a month to our error-reporting users.

This actually restores the UX from before we accepted both --shop and --store.

WHAT is this pull request doing?

Before and after:

in shopify-cli/ on resolve-s-to-store 
[°‿°]> shopify login -store=ariel-caplan-test.myshopify.com                         

Traceback (most recent call last):
	25: from /Users/amcaplan/.gem/ruby/2.7.5/bin/shopify:23:in `<main>'
	24: from /Users/amcaplan/.gem/ruby/2.7.5/bin/shopify:23:in `load'
	23: from /Users/amcaplan/.gem/ruby/2.7.5/gems/shopify-cli-2.7.4/bin/shopify:47:in `<top (required)>'
	22: from /Users/amcaplan/.gem/ruby/2.7.5/gems/shopify-cli-2.7.4/bin/shopify:36:in `block in <top (required)>'
	21: from /Users/amcaplan/.gem/ruby/2.7.5/gems/shopify-cli-2.7.4/vendor/deps/cli-kit/lib/cli/kit/error_handler.rb:21:in `call'
	20: from /Users/amcaplan/.gem/ruby/2.7.5/gems/shopify-cli-2.7.4/vendor/deps/cli-kit/lib/cli/kit/error_handler.rb:75:in `handle_abort'
	19: from /Users/amcaplan/.gem/ruby/2.7.5/gems/shopify-cli-2.7.4/bin/shopify:37:in `block (2 levels) in <top (required)>'
	18: from /Users/amcaplan/.gem/ruby/2.7.5/gems/shopify-cli-2.7.4/lib/shopify_cli/core/entry_point.rb:23:in `call'
	17: from /Users/amcaplan/.gem/ruby/2.7.5/gems/shopify-cli-2.7.4/lib/shopify_cli/core/monorail.rb:30:in `log'
	16: from /Users/amcaplan/.gem/ruby/2.7.5/gems/shopify-cli-2.7.4/lib/shopify_cli/core/entry_point.rb:24:in `block in call'
	15: from /Users/amcaplan/.gem/ruby/2.7.5/gems/shopify-cli-2.7.4/lib/shopify_cli/core/executor.rb:15:in `call'
	14: from /Users/amcaplan/.gem/ruby/2.7.5/gems/shopify-cli-2.7.4/vendor/deps/cli-kit/lib/cli/kit/executor.rb:43:in `with_traps'
	13: from /Users/amcaplan/.gem/ruby/2.7.5/gems/shopify-cli-2.7.4/vendor/deps/cli-kit/lib/cli/kit/executor.rb:55:in `twrap'
	12: from /Users/amcaplan/.gem/ruby/2.7.5/gems/shopify-cli-2.7.4/vendor/deps/cli-kit/lib/cli/kit/executor.rb:44:in `block in with_traps'
	11: from /Users/amcaplan/.gem/ruby/2.7.5/gems/shopify-cli-2.7.4/vendor/deps/cli-kit/lib/cli/kit/executor.rb:55:in `twrap'
	10: from /Users/amcaplan/.gem/ruby/2.7.5/gems/shopify-cli-2.7.4/vendor/deps/cli-kit/lib/cli/kit/executor.rb:45:in `block (2 levels) in with_traps'
	 9: from /Users/amcaplan/.gem/ruby/2.7.5/gems/shopify-cli-2.7.4/lib/shopify_cli/core/executor.rb:16:in `block in call'
	 8: from /Users/amcaplan/.gem/ruby/2.7.5/gems/shopify-cli-2.7.4/vendor/deps/cli-kit/lib/cli/kit/executor.rb:35:in `with_logging'
	 7: from /Users/amcaplan/.gem/ruby/2.7.5/gems/shopify-cli-2.7.4/vendor/deps/cli-ui/lib/cli/ui.rb:176:in `log_output_to'
	 6: from /Users/amcaplan/.gem/ruby/2.7.5/gems/shopify-cli-2.7.4/vendor/deps/cli-kit/lib/cli/kit/executor.rb:36:in `block in with_logging'
	 5: from /Users/amcaplan/.gem/ruby/2.7.5/gems/shopify-cli-2.7.4/vendor/deps/cli-ui/lib/cli/ui/stdout_router.rb:169:in `with_id'
	 4: from /Users/amcaplan/.gem/ruby/2.7.5/gems/shopify-cli-2.7.4/vendor/deps/cli-kit/lib/cli/kit/executor.rb:37:in `block (2 levels) in with_logging'
	 3: from /Users/amcaplan/.gem/ruby/2.7.5/gems/shopify-cli-2.7.4/lib/shopify_cli/core/executor.rb:17:in `block (2 levels) in call'
	 2: from /Users/amcaplan/.gem/ruby/2.7.5/gems/shopify-cli-2.7.4/lib/shopify_cli/command.rb:27:in `call'
	 1: from /Users/amcaplan/.gem/ruby/2.7.5/gems/shopify-cli-2.7.4/lib/shopify_cli/options.rb:19:in `parse'
/Users/amcaplan/.gem/ruby/2.7.5/gems/shopify-cli-2.7.4/lib/shopify_cli/options.rb:28:in `parse_flags': invalid option: s (OptionParser::InvalidOption)
	25: from /Users/amcaplan/.gem/ruby/2.7.5/bin/shopify:23:in `<main>'
	24: from /Users/amcaplan/.gem/ruby/2.7.5/bin/shopify:23:in `load'
	23: from /Users/amcaplan/.gem/ruby/2.7.5/gems/shopify-cli-2.7.4/bin/shopify:47:in `<top (required)>'
	22: from /Users/amcaplan/.gem/ruby/2.7.5/gems/shopify-cli-2.7.4/bin/shopify:36:in `block in <top (required)>'
	21: from /Users/amcaplan/.gem/ruby/2.7.5/gems/shopify-cli-2.7.4/vendor/deps/cli-kit/lib/cli/kit/error_handler.rb:21:in `call'
	20: from /Users/amcaplan/.gem/ruby/2.7.5/gems/shopify-cli-2.7.4/vendor/deps/cli-kit/lib/cli/kit/error_handler.rb:75:in `handle_abort'
	19: from /Users/amcaplan/.gem/ruby/2.7.5/gems/shopify-cli-2.7.4/bin/shopify:37:in `block (2 levels) in <top (required)>'
	18: from /Users/amcaplan/.gem/ruby/2.7.5/gems/shopify-cli-2.7.4/lib/shopify_cli/core/entry_point.rb:23:in `call'
	17: from /Users/amcaplan/.gem/ruby/2.7.5/gems/shopify-cli-2.7.4/lib/shopify_cli/core/monorail.rb:30:in `log'
	16: from /Users/amcaplan/.gem/ruby/2.7.5/gems/shopify-cli-2.7.4/lib/shopify_cli/core/entry_point.rb:24:in `block in call'
	15: from /Users/amcaplan/.gem/ruby/2.7.5/gems/shopify-cli-2.7.4/lib/shopify_cli/core/executor.rb:15:in `call'
	14: from /Users/amcaplan/.gem/ruby/2.7.5/gems/shopify-cli-2.7.4/vendor/deps/cli-kit/lib/cli/kit/executor.rb:43:in `with_traps'
	13: from /Users/amcaplan/.gem/ruby/2.7.5/gems/shopify-cli-2.7.4/vendor/deps/cli-kit/lib/cli/kit/executor.rb:55:in `twrap'
	12: from /Users/amcaplan/.gem/ruby/2.7.5/gems/shopify-cli-2.7.4/vendor/deps/cli-kit/lib/cli/kit/executor.rb:44:in `block in with_traps'
	11: from /Users/amcaplan/.gem/ruby/2.7.5/gems/shopify-cli-2.7.4/vendor/deps/cli-kit/lib/cli/kit/executor.rb:55:in `twrap'
	10: from /Users/amcaplan/.gem/ruby/2.7.5/gems/shopify-cli-2.7.4/vendor/deps/cli-kit/lib/cli/kit/executor.rb:45:in `block (2 levels) in with_traps'
	 9: from /Users/amcaplan/.gem/ruby/2.7.5/gems/shopify-cli-2.7.4/lib/shopify_cli/core/executor.rb:16:in `block in call'
	 8: from /Users/amcaplan/.gem/ruby/2.7.5/gems/shopify-cli-2.7.4/vendor/deps/cli-kit/lib/cli/kit/executor.rb:35:in `with_logging'
	 7: from /Users/amcaplan/.gem/ruby/2.7.5/gems/shopify-cli-2.7.4/vendor/deps/cli-ui/lib/cli/ui.rb:176:in `log_output_to'
	 6: from /Users/amcaplan/.gem/ruby/2.7.5/gems/shopify-cli-2.7.4/vendor/deps/cli-kit/lib/cli/kit/executor.rb:36:in `block in with_logging'
	 5: from /Users/amcaplan/.gem/ruby/2.7.5/gems/shopify-cli-2.7.4/vendor/deps/cli-ui/lib/cli/ui/stdout_router.rb:169:in `with_id'
	 4: from /Users/amcaplan/.gem/ruby/2.7.5/gems/shopify-cli-2.7.4/vendor/deps/cli-kit/lib/cli/kit/executor.rb:37:in `block (2 levels) in with_logging'
	 3: from /Users/amcaplan/.gem/ruby/2.7.5/gems/shopify-cli-2.7.4/lib/shopify_cli/core/executor.rb:17:in `block (2 levels) in call'
	 2: from /Users/amcaplan/.gem/ruby/2.7.5/gems/shopify-cli-2.7.4/lib/shopify_cli/command.rb:27:in `call'
	 1: from /Users/amcaplan/.gem/ruby/2.7.5/gems/shopify-cli-2.7.4/lib/shopify_cli/options.rb:19:in `parse'
/Users/amcaplan/.gem/ruby/2.7.5/gems/shopify-cli-2.7.4/lib/shopify_cli/options.rb:28:in `parse_flags': ambiguous option: -store=ariel-caplan-test.myshopify.com (OptionParser::AmbiguousOption)

in shopify-cli/ on resolve-s-to-store 
[°‿°]> bin/shopify login -store=ariel-caplan-test.myshopify.com
⭑ You are running a development version of the CLI at:
  /Users/amcaplan/src/github.com/Shopify/shopify-cli/bin/shopify

┏━━ Error ━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━
┃ ✗ Invalid store provided (tore=ariel-caplan-test.myshopify.com). Please provide the store in the following format: my-store.myshopify.com
┃ 
┗━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━

How to test your changes?

See before/after above

Update checklist

  • I've added a CHANGELOG entry for this PR (if the change is public-facing)
  • I've considered possible cross-platform impacts (Mac, Linux, Windows).
  • I've left the version number as is (we'll handle incrementing this when releasing).
  • I've included any post-release steps in the section above.

@amcaplan amcaplan requested review from a team, hannachen and jesalerno84 and removed request for a team December 22, 2021 16:45
gonzaloriestra
gonzaloriestra approved these changes Dec 22, 2021
View reviewed changes
Copy link
Contributor

@gonzaloriestra gonzaloriestra left a comment

Choose a reason for hiding this comment

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

I think the new error is still a bit confusing (Invalid store provided (tore=ariel-caplan-test.myshopify.com), but definitely better than the current one!

@amcaplan
Copy link
Contributor Author

That's exactly my thought. Baby steps, right?

Hopefully the user sees this and realizes that they were supposed to put 2 dashes. Whereas ambiguous was just completely barking up the wrong tree.

@amcaplan amcaplan merged commit 907b3c6 into main Dec 23, 2021
@amcaplan amcaplan deleted the resolve-s-to-store branch December 23, 2021 10:17
@pepicrft pepicrft mentioned this pull request Jan 6, 2022
Merged
@shopify-shipit shopify-shipit bot temporarily deployed to rubygems January 6, 2022 11:10 Inactive
@ADTC ADTC mentioned this pull request Jan 31, 2022
Closed
@ADTC
Copy link
Contributor

ADTC commented Jan 31, 2022

Just wondering, couldn't -store= self-correct itself to --store= and just warn the user that it's doing so? Anyway I believe it's impossible for any store to start with tore= so I don't see how this could cause a problem.

As for -s, it should be followed by a space, isn't it? Even if it isn't followed by a space, this self-correction should be a special case^, that doesn't normally apply to usages like -smystore.myshopify.com (which is also kinda confusing anyway).

^ only applying when the apparent store name starts with tore=, in which case, that part should be stripped off.

@amcaplan
Copy link
Contributor Author

@ADTC thanks for your question. We considered that option, but:

  1. It's not necessarily trivial to disambiguate reliably, especially noting that there are a number of corrections we're looking for.
  2. It's also not so simple to fix given the way we have OptionParser set up.
  3. We'd rather educate the user to enter the correct command next time, especially if they may have this running in e.g. CI environments.
  4. Fixing user inputs gets into the dangerous question of what happens if they didn't want us to "fix" their input. We think we know what they meant, but we could be wrong. There are many errors we've seen from users, and I wouldn't assume this "correction" will always be correct.

To answer your other question, generally in posix-style command line programs, in my experience, single-letter flags like -a=abc can also look like -a abc or -aabc and it's all accepted, though individual programs may of course vary.

For example, see this example from a blog post from CLI framework oclif:

Screen Shot 2022-01-31 at 16 16 05

Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Reviewers

@gonzaloriestra gonzaloriestra gonzaloriestra approved these changes

@hannachen hannachen Awaiting requested review from hannachen

@jesalerno84 jesalerno84 Awaiting requested review from jesalerno84

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
3 participants
@amcaplan @ADTC @gonzaloriestra