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

qa: Ignore shellcheck warning SC2236 #15164

Merged
merged 1 commit into from Jan 14, 2019

Conversation

Projects
None yet
3 participants
@promag
Copy link
Member

commented Jan 14, 2019

With shellcheck 0.6.0 the warning SC2236 - Use -n instead of ! -z is raised. This change adds that warning to the ignored list.

@laanwj

This comment has been minimized.

Copy link
Member

commented Jan 14, 2019

Thanks. utACK 98584ff548e267a1dc1d5240e560c623c5287424, let's merge if it fixes travis.

BTW, a similar thing was already done before in 908a559.

To prevent this from happening over and over again it might make sense to change to a list of checks to include, instead of a list of checks to exclude.
(not sure this is easy to do or would require manually going over the list)

@promag

This comment has been minimized.

Copy link
Member Author

commented Jan 14, 2019

@practicalswift

This comment has been minimized.

Copy link
Member

commented Jan 14, 2019

Concept ACK, but please print the version only when shellcheck had something to say by adding:

if [[ $? != 0 ]]; then
    echo
    shellcheck --version
fi

IMO checks should be silent unless deviations are found. That improves the signal to noise in the output when running multiple checks.

The tweak @laanwj is suggestion can be done in a follow-up PR.

@promag

This comment has been minimized.

Copy link
Member Author

commented Jan 14, 2019

@practicalswift what's the problem of showing the version? Then it's easier to compare between green and red builds.

@practicalswift

This comment has been minimized.

Copy link
Member

commented Jan 14, 2019

@promag Travis already prints the shellcheck version by default under "Build system information":

shellcheck version
0.6.0

But more generally we should try to keep the signal to noise as high as possible :-)

@promag

This comment has been minimized.

Copy link
Member Author

commented Jan 14, 2019

Travis already prints the shellcheck version by default under "Build system information":

Oh ty! will remove that.

qa: Ignore shellcheck warning SC2236
With shellcheck 0.6.0 the warning `SC2236 -  Use -n instead of ! -z` is raised.
This change adds that warning to the ignored list.

@promag promag force-pushed the promag:2019-01-shellcheck branch to f652f85 Jan 14, 2019

@promag

This comment has been minimized.

Copy link
Member Author

commented Jan 14, 2019

Done.

@practicalswift

This comment has been minimized.

Copy link
Member

commented Jan 14, 2019

utACK f652f85

pull bot pushed a commit to jaschadub/bitcoin that referenced this pull request Jan 14, 2019

Merge bitcoin#15164: qa: Ignore shellcheck warning SC2236
f652f85 qa: Ignore shellcheck warning SC2236 (João Barbosa)

Pull request description:

  With shellcheck 0.6.0 the warning `SC2236 -  Use -n instead of ! -z` is raised. This change adds that warning to the ignored list.

Tree-SHA512: 7b0dfbce55e5da4efb927e251257993cdf67cd1c90f8490d99fa75bf6e233bbee79ea1c59d28774994c59862c5eac061313aa154833284950fc844bda60a54e9

@laanwj laanwj merged commit f652f85 into bitcoin:master Jan 14, 2019

0 of 2 checks passed

continuous-integration/appveyor/pr Waiting for AppVeyor build to complete
Details
continuous-integration/travis-ci/pr The Travis CI build is in progress
Details

@promag promag deleted the promag:2019-01-shellcheck branch Jan 14, 2019

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.