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

Add theme (-t/--theme=NAME_OR_ID) parameter to theme push/theme pull commands #1877

Merged
merged 5 commits into from
Jan 13, 2022

Conversation

karreiro
Copy link
Contributor

@karreiro karreiro commented Dec 22, 2021

WHY are these changes introduced?

Users can't set the theme name when running commands like shopify theme push -u (#1431).

WHAT is this pull request doing?

This PR introduces a new parameter representing the theme name -t/--theme=NAME_OR_ID. So, now users can perform operations shopify theme push -u -t my_theme.

For consistency, this PR also:

  • Adds the -t/--theme=NAME_OR_ID to theme pull
  • Adds the -t/--theme=NAME_OR_ID to theme push (globally, even without the--unpublished parameter)

Thus, now users may rely on the -t <NAME_OR_ID> to push/pull a theme, instead of only relying on -i <ID>.

How to test your changes?

Run the following commands for checking the impact of the new parameter on theme push:

shopify theme push --live
shopify-dev theme push --theme abc
shopify-dev theme push -t abc
shopify-dev theme push -u -t abc

Run the following commands for checking the impact of the new parameter on theme pull:

shopify-dev theme pull --theme abc
shopify-dev theme pull -t abc

Post-release steps

Update documentation with the new -t/--theme=THEME parameter: https://shopify.dev/themes/tools/cli/theme-commands#push, https://shopify.dev/themes/tools/cli/theme-commands#pull.

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 linked an issue Jan 5, 2022 that may be closed by this pull request
@karreiro
Copy link
Contributor Author

karreiro commented Jan 10, 2022

Hi, @MeredithCastile! Thanks again for the thoughts you've put into this and for the questions you've raised. @macournoyer and I talked a bit about this PR in the latest theme meeting.

Considering some extra context for this use case:

  • the flag --themename is not intuitive
  • user's PR is not in the scope of theme CLI lifecycle in this context
  • repurposing/renaming the flag --nodelete to free up -n could be dangerous as users unaware of the change could lose files
  • keeping the paradigm of command --flags is valuable to keep the DX consistent and not break users expectation

We've reasoned that bundling the flags is a great idea 🚀

Thus, users could now rely on the -t/--theme flag to pass the theme id or the theme name. What do you think about this approach?

Thanks! :)

@MeredithCastile
Copy link
Contributor

@karreiro Seems like a good solution! Thanks. 🙌

Please do make sure that the --help and docs are updated to reflect these changes. If you run into any problems updating the command in .dev docs, I'd recommend reaching out to @shainaraskas.

@karreiro karreiro marked this pull request as ready for review January 12, 2022 13:58
@karreiro karreiro requested review from a team, pepicrft, hannachen and macournoyer and removed request for a team, pepicrft and hannachen January 12, 2022 13:58
@karreiro karreiro changed the title Add theme name (-t/--themename=THEMENAME) parameter to theme push/theme pull commands Add theme (-t/--theme=THEME) parameter to theme push/theme pull commands Jan 12, 2022
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.

I think it's confusing to have two args do the same thing now :/

-i ID and -t ID

But looks good on the code side.

lib/project_types/theme/messages/messages.rb Outdated Show resolved Hide resolved
lib/project_types/theme/messages/messages.rb Outdated Show resolved Hide resolved
parameter to `theme push`/`theme pull` commands
- `-t`/`--theme` accepts the theme ID or name
Improve `--help` from `--theme=THEME` to `--theme=NAME_OR_ID`
@karreiro karreiro changed the title Add theme (-t/--theme=THEME) parameter to theme push/theme pull commands Add theme (-t/--theme=NAME_OR_ID) parameter to theme push/theme pull commands Jan 13, 2022
@karreiro
Copy link
Contributor Author

Thanks for the suggestions, @macournoyer! I agree that having two args for the same thing is odd.

So, in a new commit, I've deprected the -i, --themeid from --help commands and also included a warning to help users during this migration:

image

warn

Also, +1 for removing it from the usage doc.

Does it sound like a good plan? :)

@macournoyer
Copy link
Contributor

Does it sound like a good plan? :)

YES! 🙏

@karreiro karreiro merged commit 757fda4 into main Jan 13, 2022
@karreiro karreiro deleted the fix-1431 branch January 13, 2022 13:31
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.

Themes - Feature Request: Add flag for setting Theme's name
3 participants