Fix RootHelper parse logic to support options with an equal (e.g.: option=value)

[karreiro]

WHY are these changes introduced?

Users may successfully run shopify-dev theme serve --live-reload off. However, they face an error when they try to run shopify-dev theme serve --live-reload=off.

WHAT is this pull request doing?

This PR slightly changes the RootHelper parse logic to support flags with an equal character between the option key and value.

How to test your changes?

• Go to a theme directory
• Run shopify theme serve --live-reload=off
• Notice the CLI no long raises an error

Before this PR:

After this PR:

Post-release steps

None.

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.

[macournoyer]

Hum, I'm not sure I understand the purpose of RootHelper, shouldn't https://ruby-doc.org/stdlib-2.7.1/libdoc/optparse/rdoc/OptionParser.html handle all of this for us? I think we're already using it for parsing too.

[karreiro]

Thanks for the review, @macournoyer!

Ideally, yes, OptionParser would take care of this for us.

However, it doesn't support glob parameters (essential for --only/--ignore flags #2066). Thus, we've introduced the RootHelper, which analyzes the parser and avoid unexpected values for root in scenarios like this:

$ shopify theme push -d --only layout/*

# args: ["layout/theme.liquid"]
# flags: {:development=>true, :only=>["layout/password.liquid"]}

Before the RootHelper, the CLI would consider the "layout/theme.liquid" as the root (as it was getting the first element of args).

Given the impact of this fix, I'm proceeding with the merge. But, please, let me know if you'd like to revisit it when you're back, @macournoyer! :)

Thanks again for the review! 🚀

[macournoyer]

But shopify theme push -d --only layout/* is expended by the shell. I think it's expected behavior. For the same reason you have to quote your glob in find . -name "*.liquid". We want the glob to be evaluated by the CLI, not the shell. And so have to use quotes.

I might not be understanding this correctly. My only concern is: are we working against the shell? I can't think of any other CLI where something like --only layout/* would work (without the quotes). Have you seen examples of this?

[karreiro]

Thanks for reviewing this PR and sharing your thoughts, @macournoyer :)

I agree that other CLI tools (as find) adopt a different approach. And I initially considered closing the ticket suggesting the use of quotes, as that's a regular behavior.

However, considering the scenario above (reported by a partner), it seems a bit unexpected that the CLI was adopting the "layout/theme.liquid" as the root and the "layout/password.liquid" as the only.

So, considering the pros and cons on both sides, it seems fair to support parameters without quotes to provide a better DX for partners that are not familiar with this kind of scenario.

Still, I'm not sure if we're missing any thread-offs or limitations in this direction. Do you have thoughts about that?

[macournoyer]

But don't you think it's more confusing if we chose a different approach than most CLIs?

It's only unexpected by people who are not aware of how shell expansion works. If a partner is confused about it, I think we should educate, instead of working around it.

Here's how tar does it too for the same use case:

tar -cf src.tar --exclude='*.o' src
-- https://www.gnu.org/software/tar/manual/html_node/exclude.html