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

Refactor Shell Scripts based on ShellCheck's suggestion #828

Merged
merged 3 commits into from Dec 30, 2017

Conversation

PeterDaveHello
Copy link
Contributor

@PeterDaveHello PeterDaveHello commented Dec 5, 2017

This will help improve the robustness of the scripts, some of the deprecated and non-safe statements need to be refactored.

@Changaco
Copy link
Member

Changaco commented Dec 5, 2017

Thanks but I'm not going to merge this as-is. The changes seem mostly unnecessary, and some of them are in obsolete files that I should have deleted months ago.

@PeterDaveHello
Copy link
Contributor Author

I can revert the changes on files need to be deleted. I don't think they are all that unnecessary, for example, one of their header used #!/bin/sh which may not support many of statements in the script, and -o/-a in bash condition were deprecated for a while, it's no hurt to have these changes, they only make those scripts less potential of issues.

@chadwhitacre
Copy link
Contributor

chadwhitacre commented Dec 30, 2017

Merging the changes to the obsolete files makes no difference since the files are obsolete anyway.

Merging the unnecessary changes probably(?) won't hurt.

I agree that @PeterDaveHello's PRs such as this one tend to be low-value, but ¯\_(ツ)_/¯. Seems best to either merge or close to keep the PR queue moving.

@Changaco
Copy link
Member

Rebased on master and added a commit to delete the obsolete files.

@Changaco
Copy link
Member

And another to revert changes that weren't merely unnecessary but invalid.


# Fail on errors and undefined variables
set -eu

# Be somewhere predictable
cd "`dirname $0`"
cd "$(dirname "$0")"
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I don't know how the double quotes behave here.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is quite common in shell scripts, I think it works as intended.

@chadwhitacre
Copy link
Contributor

Assuming those double double quotes lines work as intended, lgtm.

@Changaco Changaco merged commit 35e03d5 into liberapay:master Dec 30, 2017
@PeterDaveHello PeterDaveHello deleted the ShellScriptRefactor branch January 5, 2018 17:03
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants