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
style: set shell variables in hash #7696
Conversation
3633977
to
f3148fe
Compare
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.
Looking good so far!
f0d9840
to
bd5bd54
Compare
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.
Again: great work so far 👏
bd5bd54
to
394fae1
Compare
Todo:
|
9de4813
to
a89936b
Compare
bbce16e
to
606db89
Compare
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.
Looking good so far! One question.
606db89
to
ce00713
Compare
When running Utils.popen (or similar popen command), having a line like "SHELL=bash ..." doesn't work properly. Instead, use: `Utils.popen({ "SHELL" => "bash" }, "...")`
ce00713
to
e820845
Compare
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.
Looks good, nice work again @Rylan12!
brew style
with your changes locally?brew tests
with your changes locally?When running Utils.popen (or similar popen command), having a line like "SHELL=bash ..." doesn't work properly. Instead, using `Utils.popen({ "SHELL" => "bash" }, "...") works correctly.
See Homebrew/homebrew-core#55682 for more info. This comment may also be helpful.
Example:
Utils.safe_popen_read "SHELL=bash foo"
becomesUtils.safe_popen_read({ "SHELL" => "bash" }, "foo")
Here is a concern of mine: in doitlive, running the audit on the line
says to change to
when the correct line is actually
I think this is okay because, once #7691 and #7695 are merged, separate audit statements will recomend changing
popen_read
tosafe_popen_read
, and separating#{libexec}/bin/doitlive completion
into its individual arguments.These formulae will need to be updated to match this new style: