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.
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
feat: install dependencies in .tool-versions order #1723
base: master
Are you sure you want to change the base?
feat: install dependencies in .tool-versions order #1723
Changes from 2 commits
7c7232c
05718d4
cf718b2
1bd6cad
6693274
b4d8306
675dc7f
cea75a6
f6fd11a
54379a5
596a1ea
0acb232
1e2d698
f1a268e
c684c1a
3d97f31
53998e2
257fff6
8d0fc4f
File filter
Filter by extension
Conversations
Jump to
There are no files selected for viewing
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think the
$()
on lines 122 and 124 should be quoted.There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
quote this one too I think.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This one as well. paths can contain spaces but they should be treated like a single string.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Probably want this
$()
to be quoted right?There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I am not sure to be honest. I noticed that most of the assignments in the
installs.bash
file do not put quotes around the$()
. When I ran the linter, the linter pulled up times where I had passed a$()
to a bash function if I didn't wrap it in quotes. It would give the following warning:SC2046 (warning): Quote this to prevent word splitting.
. This makes me think that variable assignment is okay but potentially passing as arguments without wrapping in quotes is the one that isn't right. Not sure though, no expert on bash myself.There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Typically if the value produced by the expression contains spaces and you want it to stay as a string you should quote -
string_with_spaces="$(echo "string with spaces")"
. I think that is the case here, but I may be wrong. Generally, quoting is safer in Bash (unless you are dealing with an array, in which case it won't work).