Update bash ex-scripts to be linter compliant #4151
Conversation
Exports variables reported by shellcheck as being unused. A more careful examination is needed to remove unnecessary variables.
WalterKolczynski-NOAA
requested review from
BoCui-NOAA,
BrianCurtis-NOAA,
CatherineThomas-NOAA,
ChristopherHill-NOAA,
CoryMartin-NOAA,
DavidHuber-NOAA,
EdwardSafford-NOAA,
EricSinsky-NOAA,
JessicaMeixner-NOAA,
RussTreadon-NOAA,
WenMeng-NOAA,
aerorahul and
sbanihash
as code owners
JessicaMeixner-NOAA
left a comment
JessicaMeixner-NOAA
left a comment
There was a problem hiding this comment.
Can someone point me to where 4 spaces instead of 2 is the standard for the g-w or some linter check? I was unaware that was a thing, since 2 spaces is very widely used across the g-w.
This PR needs to be tested on WCOSS2 to ensure that all wave AWIPS, etc jobs are still running as expected as this changes those scripts and I think most of the changes were spaces, but I'm still going through to confirm as github isn't showing all of them as such. Additionally, wave jobs should be checked that the contents is the same before and after this PR as there were a few places where commands were changed.
@WalterKolczynski-NOAA I think that this comes from the change you made to .editorconfig: |
So this is a new thing. This even seems inconsistent in .editorconfig as well. Is .editorconfig where we should be looking for these sorts of standards in the future? |
Yes. I will note before this point, no standard was enforced for bash scripts, which is why different scripts had different indentation. This PR is part of applying a standard to all the legacy scripts so enforcement can begin. Four spaces is a compromise between space usage and visibility, and is consistent with the python scripts. |
| if [ $CFP_MP = "YES" ]; then | ||
| nm=0 | ||
| fi |
There was a problem hiding this comment.
Is this set at the JJob level?
There was a problem hiding this comment.
It's not used anymore with the switch to using run_mpmd.sh.
| for filenc4 in diag*nc4.gz; do | ||
| netcdf=1 | ||
| file=$(echo "${filenc4}" | cut -d'.' -f1-2).gz | ||
| mv "${filenc4}" "${file}" | ||
| done |
There was a problem hiding this comment.
This needs a file existence check. If there are no diag*nc4.gz files, then filenc4 will be assigned the literal diag*nc4.gz.
There was a problem hiding this comment.
Actually, I think this should be reverted to the previous logic. There are some variables that could be removed (data_available, for instance), but otherwise I don't think this script should change logic.
There was a problem hiding this comment.
Shellcheck doesn't like looping over ls: https://www.shellcheck.net/wiki/SC2045
Recommend we turn on shopt -s nullglob (as noted on that page) anyway for all our code to prevent * being treated as literal anywhere.
aerorahul
left a comment
aerorahul
left a comment
There was a problem hiding this comment.
looks ok to me. just a few comments.
thanks for this. Next, we need a github action that prevents code from getting in that does not conform to the standards spelled out in editorconfig.
4 participants
Description
Updates the ex-scripts to be compliant with both
shellcheckandshfmt. This does not necessarily enforce some standards not checked byshellcheck. Export statements are added to some variables to avoid 'not used' errors. In the future, a more careful analysis should be done to see if these variables are used downstream by executables or if they are truly unused, in which case they should be removed.Scripts employing MPMD are also updated to use
run_mpmd.sh. This resulted in some retooling of those scripts.Refs: #397
Type of change
Change characteristics
How has this been tested?
Checklist