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

Apply final set of Shellcheck fixes and turn on in CI #7832

Merged
merged 16 commits into from
Jun 3, 2019

Conversation

Eric-Arellano
Copy link
Contributor

Builds off of the work from #7698 and #7719.

Several of the problems are ignored because they are very difficult to correctly fix, and the behavior is not broken when ran locally and in CI so the cost of fixing is not deemed worth it.

@@ -6,4 +6,4 @@
# Check for copies (-C) and moves (-M), so we don't get false positives when people do
# refactorings. -l50 bounds the time git takes to search for these non-additions.
# See git-diff(1) and https://stackoverflow.com/a/2299672/2518889 for discussion of these options.
exec git diff --cached --name-only --diff-filter=A -C -M -l50
exec git --no-pager diff --cached --name-only --diff-filter=A -C -M -l50
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Depending on how the user has their git set up, this script would sometimes fail. For example, my git for some reason defaults to opening git diff through less.

Now, the script will work no matter what.

Copy link
Contributor

Choose a reason for hiding this comment

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

I didn't know about this argument! Note that many programs (including git) will respect the PAGER env var, and I've usually done this by running the command with PAGER=cat -- but this is definitely better!

build-support/bin/native/bootstrap_rust.sh Outdated Show resolved Hide resolved
pants Show resolved Hide resolved
src/docs/publish_via_git.sh Show resolved Hide resolved
build-support/bin/check_rust_formatting.sh Show resolved Hide resolved
@@ -6,4 +6,4 @@
# Check for copies (-C) and moves (-M), so we don't get false positives when people do
# refactorings. -l50 bounds the time git takes to search for these non-additions.
# See git-diff(1) and https://stackoverflow.com/a/2299672/2518889 for discussion of these options.
exec git diff --cached --name-only --diff-filter=A -C -M -l50
exec git --no-pager diff --cached --name-only --diff-filter=A -C -M -l50
Copy link
Contributor

Choose a reason for hiding this comment

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

I didn't know about this argument! Note that many programs (including git) will respect the PAGER env var, and I've usually done this by running the command with PAGER=cat -- but this is definitely better!

build-support/bin/native/bootstrap_rust.sh Outdated Show resolved Hide resolved
build-support/bin/check_rust_formatting.sh Show resolved Hide resolved
build-support/bin/release.sh Outdated Show resolved Hide resolved
src/docs/publish_via_git.sh Show resolved Hide resolved
build-support/bin/native/bootstrap_rust.sh Outdated Show resolved Hide resolved
src/docs/publish_via_git.sh Show resolved Hide resolved
Copy link
Contributor

@blorente blorente left a comment

Choose a reason for hiding this comment

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

I'm learning a whole lot with these changes :) Thanks!

build-support/bin/check_rust_formatting.sh Show resolved Hide resolved
build-support/bin/native/bootstrap_rust.sh Outdated Show resolved Hide resolved
pants Show resolved Hide resolved
@Eric-Arellano Eric-Arellano merged commit e2b18b9 into pantsbuild:master Jun 3, 2019
@Eric-Arellano Eric-Arellano deleted the shellcheck-round-3 branch June 3, 2019 21:30
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.

4 participants