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

Remove back-ticks from output strings #601

Closed
mkevinosullivan opened this issue May 14, 2020 · 4 comments · Fixed by #1135
Closed

Remove back-ticks from output strings #601

mkevinosullivan opened this issue May 14, 2020 · 4 comments · Fixed by #1135
Assignees
Labels
oss:good first issue Good for newcomers

Comments

@mkevinosullivan
Copy link
Contributor

Issue summary

Remove back-ticks from output strings, especially if the end-user is expected to reproduce on the command line, as instructed by a help output. This should be attempted once PR #587 is completed (output strings will be centralized in messages.rb files).

Actual behavior

On Unix-based systems, back-ticks cause the content to be executed, so (as an example) the following output string from shopify help load-dev is mis-leading, as it can be easily interpreted for the developer to include the back-ticks in the command, which is not intended.

% shopify help load-dev
Load a development instance of Shopify App CLI from the given path. This command is intended for development work on the CLI itself.
  Usage: shopify load-dev `/absolute/path/to/cli/instance`
%

In this case, it would be advisable to remove the back-ticks altogether; in other cases (usually to highlight a piece of text), it would be better to find an alternative to do so (e.g., bold, different colour, etc.).

@mkevinosullivan mkevinosullivan added the oss:good first issue Good for newcomers label May 14, 2020
@cixtor
Copy link

cixtor commented Sep 2, 2020

Hello @mkevinosullivan, it looks like @paulomarg already removed the extraneous back-ticks with commit 4e3d52f but forgot to close this issue. This comment also acts as a public service announcement for people coming from the website Good First Commit of the Week.

@paulomarg
Copy link
Contributor

Hey @cixtor! I think the idea behind this issue is to go over the messages in general and look for instances of this issue. I most likely did not catch all of them when I made my change, since I was changing quite a few things.

@cixtor
Copy link

cixtor commented Sep 2, 2020

I already scanned the entire project at commit 41b4963 and found 1,249 instances of the extraneous back-ticks. However, 210 are in comments, 252 are in Markdown files, 770 are in test/fixtures/shopify_schema.json, 4 are in lib/docgen/class_template.md.erb, 1 is in an HTML file, the ones that are left are listed below:

Filename Line Content
vendor/deps/cli-ui/lib/cli/ui/stdout_router.rb 157 on_streams must be an array of objects that respond to `write`
lib/shopify-cli/packager.rb 102 You can install it by running `#{installation_cmd}`.
lib/shopify-cli/messages/messages.rb 65 repo_not_initiated: "Git repo is not initiated. Please run `git init` and make at least one commit.",
lib/shopify-cli/messages/messages.rb 93 could_not_select_app: "Heroku app `%s` could not be selected",
lib/project_types/rails/messages/messages.rb 88 authenticated_with_account: "{{v}} Authenticated with Heroku as `%s`",
lib/project_types/rails/messages/messages.rb 108 branch_selected: "{{v}} Git branch `%s` selected for deploy",
lib/project_types/rails/messages/messages.rb 114 selecting: "Selecting Heroku app `%s`...",
lib/project_types/rails/messages/messages.rb 115 selected: "{{v}} Heroku app `%s` selected",
lib/project_types/node/messages/messages.rb 61 authenticated_with_account: "{{v}} Authenticated with Heroku as `%s`",
lib/project_types/node/messages/messages.rb 68 branch_selected: "{{v}} Git branch `%s` selected for deploy",
lib/project_types/node/messages/messages.rb 74 selecting: "Selecting Heroku app `%s`…",
lib/project_types/node/messages/messages.rb 75 selected: "{{v}} Heroku app `%s` selected",

@paulomarg
Copy link
Contributor

Great job collecting all of these! All of the backticks on the messages.rb files in your table can be dropped - those are the files that contain the actual messages to be displayed by the CLI and are the most likely to lead to problems.

Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
oss:good first issue Good for newcomers
Projects
None yet
Development

Successfully merging a pull request may close this issue.

4 participants