Skip to content
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

Merged
merged 2 commits into from Jul 26, 2017
Merged

Cleanup maintainer script gnome.sh #23750

merged 2 commits into from Jul 26, 2017

Conversation

butterflya
Copy link
Contributor

No functional changes

Motivation for this change

Use quotation when needed.

Things done
  • Tested using sandboxing
    (nix.useSandbox on NixOS,
    or option build-use-sandbox in nix.conf
    on non-NixOS)
  • Built on platform(s)
    • NixOS
    • macOS
    • Linux
  • Tested compilation of all pkgs that depend on this change using nix-shell -p nox --run "nox-review wip"
  • Tested execution of all binary files (usually in ./result/bin/)
  • Fits CONTRIBUTING.md.

No functional changes
@mention-bot
Copy link

@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
Copy link
Member

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…

Copy link
Contributor Author

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.)

Copy link
Member

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.

Copy link
Contributor Author

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"
Copy link
Member

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.

Copy link
Contributor Author

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.

@grahamc grahamc merged commit de8b6dd into NixOS:master Jul 26, 2017
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

5 participants