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

Improve --only/--ignore parameters on Theme pull/push commands to work without quotes #2066

Merged
merged 9 commits into from
Feb 22, 2022

Conversation

karreiro
Copy link
Contributor

@karreiro karreiro commented Feb 16, 2022

WHY are these changes introduced?

The theme push command doesn't work as expected when partners use glob parameters like this: shopify theme push -d --only layout/*. However, it works when they run the same command with quotes shopify theme push -d --only "layout/*".

WHAT is this pull request doing?

Here's what happens from the ShopifyCLI::Options perspective when partners run the commands mentioned above:

$ shopify theme push -d --only "layout/*"
args: []
flags: {:development=>true, :only=>["layout/*"]}

$ shopify theme push -d --only layout/*
args: ["layout/theme.liquid"]
flags: {:development=>true, :only=>["layout/password.liquid"]}

This PR addresses this issue by relying on custom conversions to handle glob values in the Theme::Command::Pull and Theme::Command::Push classes.

How to test your changes?

  • Open a theme
  • Change something in the layout directory
  • Run shopify theme push -d --only layout/* (without quotes)
  • Notice that now the --only parameter works as expected

Before this PR
Screenshot 2022-02-16 at 17 27 55

After this PR
Screenshot 2022-02-16 at 17 03 11

Post-release steps

The documentation of --only and --ignore may mention that users can pass parameters with or without quotes.

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.

@karreiro karreiro requested review from macournoyer, a team, gonzaloriestra and molly-yu and removed request for a team February 16, 2022 16:57
@karreiro karreiro linked an issue Feb 16, 2022 that may be closed by this pull request
2 tasks
Copy link
Contributor

@macournoyer macournoyer left a comment

Choose a reason for hiding this comment

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

TIL about custom conversions!

@karreiro
Copy link
Contributor Author

(rebasing the branch)

Thanks again for your PR (#2051), @kilgore5! Just an FYI, I've updated theme serve here, following your PR and also using the new RootHelper 🚀

@karreiro karreiro merged commit 19ba80f into main Feb 22, 2022
@karreiro karreiro deleted the fix-2028 branch February 22, 2022 09:55
@kilgore5
Copy link
Contributor

kilgore5 commented Feb 22, 2022

@karreiro Awesome, thanks!

kilgore5 pushed a commit to kilgore5/shopify-cli that referenced this pull request Mar 2, 2022
After Shopify#2066, specifying multiple flags stopped working, but specifying multiple patterns for one flag is working
so easiest I think to just change the documentation to indicate how the options should be used
@pm-zr pm-zr mentioned this pull request Mar 3, 2022
2 tasks
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

[Bug]: shopify theme push --only doesn't work
3 participants