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
Cleanup maintainer script gnome.sh #23750
Conversation
No functional changes
@butterflya, thanks for your PR! By analyzing the history of the files in this pull request, we identified @urkud and @lethalman to be potential reviewers. |
echo >&2 | ||
fi | ||
done | ||
else | ||
project="$2" | ||
majorVersion="$3" | ||
project=$2 |
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 assumes that $project
will be one shell word…
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.
Can you prove that? (This is a rhetorical question, since your statement is false.)
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.
Well, because if you start caring about the quoting, it is bad style not to quote multi-word variable anywhere? Even if it happens to work in the specific case.
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.
You meant everywhere instead of anywhere? It's your opinion that this is bad style. All you need to do is understand the rules. If the style is that correctness is the most important quality combined with a minimum number of characters under the condition that variables can contain spaces (the general rule is more complicated), then my style (which happens to coincide with the style of experts) is the one to follow.
In the past few years some silly megacorporations have written style guides, which contradict this style in order to save a few dollars on staffing costs (because ignorant people are cheaper). In many cases the style guides have been written by people who themselves don't know what they are doing. I'd prefer it if we did not have people touching code who have to be explained this.
To be honest, I think it's a bit silly to waste my time on even explaining this stuff. My version is more correct that the original version. Why don't you just merge the commit instead of starting a non-productive discussion?
if [ "$action" = show ]; then | ||
show_project "$project" "$majorVersion" | ||
elif [ "$action" = update ]; then | ||
update_project "$project" "$majorVersion" |
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.
…And this admits it can be multiple words. I do not really understand the logic of this cleanup.
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.
Indeed, this allows a project to be of the form 'my helloworld project', exactly as intended.
No functional changes
Motivation for this change
Use quotation when needed.
Things done
(nix.useSandbox on NixOS,
or option
build-use-sandbox
innix.conf
on non-NixOS)
nix-shell -p nox --run "nox-review wip"
./result/bin/
)