Skip to content

qa: Ignore shellcheck warning SC2236 #15164

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.

Sign up for GitHub

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

Jump to bottom
Merged
merged 1 commit into from
Jan 14, 2019
Merged

qa: Ignore shellcheck warning SC2236 #15164

merged 1 commit into from
Jan 14, 2019

Conversation

promag
Copy link
Contributor

@promag promag commented Jan 14, 2019 •
edited
Loading

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

laanwj 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
Copy link
Contributor Author

promag commented Jan 14, 2019

@practicalswift ping.

@practicalswift
Copy link
Contributor

practicalswift commented Jan 14, 2019 •
edited
Loading

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
Copy link
Contributor Author

promag 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
Copy link
Contributor

practicalswift commented Jan 14, 2019 •
edited
Loading

@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
Copy link
Contributor Author

promag commented Jan 14, 2019

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

Oh ty! will remove that.

@promag
qa: Ignore shellcheck warning SC2236
f652f85 
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 2019-01-shellcheck branch from 98584ff to f652f85 Compare January 14, 2019 15:22
@promag
Copy link
Contributor Author

promag commented Jan 14, 2019

Done.

@practicalswift
Copy link
Contributor

utACK f652f85

pull bot pushed a commit to jaschadub/bitcoin that referenced this pull request Jan 14, 2019
@laanwj
Merge bitcoin#15164: qa: Ignore shellcheck warning SC2236
035f349 
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
@promag promag deleted the 2019-01-shellcheck branch January 14, 2019 15:26
@practicalswift practicalswift mentioned this pull request Jan 14, 2019
Merged
@str4d str4d mentioned this pull request Oct 27, 2020
Merged
zkbot added a commit to zcash/zcash that referenced this pull request Oct 28, 2020
@zkbot
Auto merge of #4829 - str4d:backport-lints, r=<try>
43ac206 
Backport useful lints from upstream

Cherry-picked from the following upstream PRs:
- bitcoin/bitcoin#6892
- bitcoin/bitcoin#11151
- bitcoin/bitcoin#11300
- bitcoin/bitcoin@96d91b7
- bitcoin/bitcoin#12097
- bitcoin/bitcoin#12098
- bitcoin/bitcoin#12442
- bitcoin/bitcoin#12572
- bitcoin/bitcoin#12757
- bitcoin/bitcoin#11878
- bitcoin/bitcoin#12933
- bitcoin/bitcoin#12871
- bitcoin/bitcoin#12972
- bitcoin/bitcoin#13281
- bitcoin/bitcoin#13385
- bitcoin/bitcoin#13041
- bitcoin/bitcoin#13454
- bitcoin/bitcoin#13448
- bitcoin/bitcoin#13510
- bitcoin/bitcoin#13851
- bitcoin/bitcoin#13863
- bitcoin/bitcoin#14115
- bitcoin/bitcoin#14831
- bitcoin/bitcoin#15164
- bitcoin/bitcoin#15170
- bitcoin/bitcoin#15166
- bitcoin/bitcoin#16036
- bitcoin/bitcoin#16768

Several of the lints fail for our current codebase; these will be addressed in a
subsequent PR.
zkbot added a commit to zcash/zcash that referenced this pull request Nov 9, 2020
@zkbot
Auto merge of #4829 - str4d:backport-lints, r=str4d
84a5830 
Backport useful lints from upstream

Cherry-picked from the following upstream PRs:
- bitcoin/bitcoin#6892
- bitcoin/bitcoin#11151
- bitcoin/bitcoin#11300
- bitcoin/bitcoin@96d91b7
- bitcoin/bitcoin#12097
- bitcoin/bitcoin#12098
- bitcoin/bitcoin#12442
- bitcoin/bitcoin#12572
- bitcoin/bitcoin#12757
- bitcoin/bitcoin#11878
- bitcoin/bitcoin#12933
- bitcoin/bitcoin#12871
- bitcoin/bitcoin#12972
- bitcoin/bitcoin#13281
- bitcoin/bitcoin#13385
- bitcoin/bitcoin#13041
- bitcoin/bitcoin#13454
- bitcoin/bitcoin#13448
- bitcoin/bitcoin#13510
- bitcoin/bitcoin#13851
- bitcoin/bitcoin#13863
- bitcoin/bitcoin#14115
- bitcoin/bitcoin#14831
- bitcoin/bitcoin#15164
- bitcoin/bitcoin#15170
- bitcoin/bitcoin#15166
- bitcoin/bitcoin#16036
- bitcoin/bitcoin#16768
- bitcoin/bitcoin#13494

Several of the lints fail for our current codebase; these will be addressed in a
subsequent PR.
PastaPastaPasta pushed a commit to PastaPastaPasta/dash that referenced this pull request Jun 26, 2021
@laanwj @PastaPastaPasta
Merge bitcoin#15164: qa: Ignore shellcheck warning SC2236
d84c8be 
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
PastaPastaPasta pushed a commit to PastaPastaPasta/dash that referenced this pull request Jun 28, 2021
@laanwj @PastaPastaPasta
Merge bitcoin#15164: qa: Ignore shellcheck warning SC2236
de60193 
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
@bitcoin bitcoin locked as resolved and limited conversation to collaborators Dec 16, 2021
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Reviewers
No reviews
Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
3 participants
@promag @laanwj @practicalswift