training-manual
training-manual copied to clipboard
githubtraining
Feedback please: Code quality checks of scripts
Running shellcheck to check the code quality of the scripts in the script directory result in over 580 code quality issues across 12 scripts. These issues can have unintended consequences that will result in problems with the execution of the various scripts.
test.txt (rename test.txt to test or test.bash, make executable, run; requires shellcheck - See https://github.com/koalaman/shellcheck)
I'm willing to undertake this issue and resolve the code quality issues if it is considered valuable.
Feedback please.
Hello @smokeybreeze! The current use of the scripts is during class, to understand how students are doing. I think that their biggest value is for remote classes. I know that I would find any improvement valuable, and I'm sure other trainers would too. However, I don't know if it's critical or in the way of delivering courses as they are.
That being said, if you have the bandwidth to improve them, your contributions would be very appreciated. 👍
@brianamarie, we have a similar thread going in our GHE instance. Here's what I posted as feedback to @smokeybreeze:
I scanned a bit of the file. Some are legitimate, and some aren't. We've done functional testing to ensure that this works on bash and git bash. Are you sure posix sh is the correct thing to validate against? I wouldn't want to make a lot of these fixes because they'd break base functionality in some cases.
My main concern is that @mblack88 and I did a bunch of changes and testing to make sure that things ran correctly in both bash and git bash. I'm wondering if we change from sh to bash explicitly and then rerun using bash validation instead of posix validation. Having way more code quality issues than lines of code seems excessive when the scripts work. Is there a reason why zsh is supported? That support is the only reason why I can see using sh instead of bash in the script header. If we only need to support bash, I see things becoming much simpler.
We can significantly reduce the number of issues by replacing the #!/bin/sh shebang with #!/bin/bash. The shebang tells the operating system what interpreter to use to execute the script. If we changed the shebang to #!/bin/bash, the issues that remain are mostly related to double quoting variables to avoid globbing and word splitting which would not alter the current functionality of the scripts.
A lot of git users prefer the Z shell (zsh) because it supports git command expansion, so I think keeping support for adding "source $HOME/.trainingmanualrc" to both ~/.zshrc and ~/.bashrc is valuable, but that is a different user story altogether.
As Amy mentioned we spent a good amount of time to make sure the current scripts work for both major platforms, windows and Mac. My opinion is that the scripts run with the “basic” install of “git” for each platform and stay that generic. The other solution maybe to creat containers that can run on each platform to remove dependencies that might not exist on someone’s local system.
Sent from my iPhone
On Jan 25, 2019, at 11:56 AM, smokeybreeze <[email protected]mailto:[email protected]> wrote:
We can significantly reduce the number of issues by replacing the #!/bin/sh shebang with #!/bin/bash. The shebang tells the operating system what interpreter to use to execute the script. If we changed the shebang to #!/bin/bash, the issues that remain are mostly related to double quoting variables to avoid globbing and word splitting which would not alter the current functionality of the scripts.
A lot of git users prefer the Z shell (zsh) because it supports git command expansion, so I think keeping support for adding "source $HOME/.trainingmanualrc" to both ~/.zshrc and ~/.bashrc is valuable, but that is a different user story altogether.
— You are receiving this because you were mentioned. Reply to this email directly, view it on GitHubhttps://github.com/githubtraining/training-manual/issues/168#issuecomment-457640127, or mute the threadhttps://github.com/notifications/unsubscribe-auth/AU77UAddJMhqVVR8pDgm9JavD9JEGc3-ks5vGzc9gaJpZM4aRqzf.
I think Amy's suggestion to target bash will make maintenance much easier and will still work for zsh user's assuming they haven't completely uninstalled the bash shell from their machine (rare due to obviouse compatibility issues that this causes).
On January 25, 2019 11:31:47 AM CST, Matt Blackburn [email protected] wrote:
As Amy mentioned we spent a good amount of time to make sure the current scripts work for both major platforms, windows and Mac. My opinion is that the scripts run with the “basic” install of “git” for each platform and stay that generic. The other solution maybe to creat containers that can run on each platform to remove dependencies that might not exist on someone’s local system.>
Sent from my iPhone>
On Jan 25, 2019, at 11:56 AM, smokeybreeze <[email protected]mailto:[email protected]> wrote:>
We can significantly reduce the number of issues by replacing the #!/bin/sh shebang with #!/bin/bash. The shebang tells the operating system what interpreter to use to execute the script. If we changed the shebang to #!/bin/bash, the issues that remain are mostly related to double quoting variables to avoid globbing and word splitting which would not alter the current functionality of the scripts.>
A lot of git users prefer the Z shell (zsh) because it supports git command expansion, so I think keeping support for adding "source $HOME/.trainingmanualrc" to both ~/.zshrc and ~/.bashrc is valuable, but that is a different user story altogether.>
—> You are receiving this because you were mentioned.> Reply to this email directly, view it on GitHubhttps://github.com/githubtraining/training-manual/issues/168#issuecomment-457640127, or mute the threadhttps://github.com/notifications/unsubscribe-auth/AU77UAddJMhqVVR8pDgm9JavD9JEGc3-ks5vGzc9gaJpZM4aRqzf.>
-- > You are receiving this because you are subscribed to this thread.> Reply to this email directly or view it on GitHub:> https://github.com/githubtraining/training-manual/issues/168#issuecomment-457651306
-- Sent from my Android device with K-9 Mail. Please excuse my brevity.
The scripts are written using the Bourne Again SHell extensions. so setting the shebang to #!/bin/bash would be most accurate. Setting the shebang to the POSIX Bourne shell (#!/bin/sh) and not adhering to what is actually in the POSIX standard is relying on the operating system vendors to not strictly adhere to the POSIX standard. [[ ]] constructs, arrays, and the function keyword are just some bash extensions that are not provided in the POSIX standard, so technically these scripts should not run as written.
The bottom line is that we've already standardized on bash for these scripts.
Apologies for my lack of depth in knowledge in Bash scripting - it sounds to me like @smokeybreeze, you're suggesting that we standardize to #!/bin/bash. Is that correct? If so, would that introduce any problems with the Windows (Git Bash) usability?
We already have standardized on Bash based on the language constructs within the scripts.
To answer your question directly, it shouldn't create any issues with the Windows git bash since the Windows git bash actually uses the same GNU Bash that Mac OS X does, but the Windows git bash version is newer than the verstion of GNU Bash that comes on MAC OS X.