scope launch script improvements #2077
Merged
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
ekimekim
force-pushed
the
mike/scope-script/shellcheck
branch
from
565f499 to
82270b0
Compare
|
This fixes #2073, right? And perhaps some others? |
|
yes, sorry. I edited to mention that between you loading and you commenting. |
|
Slightly changed the wording to properly fix #1556 |
jml
approved these changes
Dec 12, 2016
| # TODO: properly escape/quote the output of "$@" | ||
| echo $(launch_command) "$@" | ||
| # Most systems should have printf, but the %q specifier isn't mandated by posix | ||
| # and can't be guarenteed. Since this is mainly a cosmetic output and the alternative |
This comment was marked as abuse.
This comment was marked as abuse.
Sorry, something went wrong.
This comment was marked as abuse.
This comment was marked as abuse.
Sorry, something went wrong.
This comment was marked as abuse.
This comment was marked as abuse.
Sorry, something went wrong.
By far the majority of these were variables which were not quoted. While, yes, right now we can guarentee most of these variables will never contain spaces, this could someday change and applying quoting as a universal rule prevents future mistakes. The ARGS="$@" -> "$*" change is purely stylistic and mainly is used to indicate the intent that we actually wanted to concatenate all the args by spaces, not keep them seperated as "$@" would in many situations, but not this one. Several warnings remain, in places where we intentionally want to split a variable on whitespace, or otherwise do what shellcheck is warning us against. Of note is shellcheck warning SC2166, which says to prefer [ foo ] || [ bar ] over [ foo -o bar ] as the -a and -o flags have differing behaviour on some systems. I've opted to keep these for now, since the version check test command would need to be replaced by a LOT of subshells to achieve the same effect, which feels dirtier.
As indicated by the TODO, any args passed into the command do not get escaped when output, so for example: scope command "foo bar" would output results like: foo bar instead of "foo bar" or foo\ bar The "right" way to do this seems to be printf %q, which prints a quoted version of the string. However this format specifier is not available in POSIX sh (though it does work in many implementations of it, such as the ones provided by bash which make up the likely majority of real-world usage). This code is a compromise that uses the added functionality where available, while still falling back to the old behaviour when it isn't.
Instead of different usage info for "scope help", show the same always. Also correct it for what the script actually does, and always display the scope binary args.
It's not useful and it's confusing. Replaced with a generic "it worked" message.
… worked We can't easily do this since scope exits failure when -h is passed, so we can't distinugish between success and failure.
ekimekim
force-pushed
the
mike/scope-script/shellcheck
branch
from
17e83ff to
2562567
Compare
Closed
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
scope commandwhen availableSee individual commits for discussion on each point.
Fixes #2073. Fixes #1556. Fixes #1557. Fixes #1560.