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

"Shell" should set some standards #524

Open
Gorian opened this issue Nov 28, 2020 · 2 comments
Open

"Shell" should set some standards #524

Gorian opened this issue Nov 28, 2020 · 2 comments

Comments

@Gorian
Copy link

Gorian commented Nov 28, 2020

I looked at a few of the "shell scripting" tutorials, and a few things immediately jumped out at me.

  • You should either commit to teach POSIX compliant shell scripting, or to teach bash specifically. You shouldn't mix the two, like using bash to write POSIX code in one tutorial, and then have the next section be about arrays, which are a bashism and not POSIX compliant
  • All code should be https://shellcheck compliant - note that shellcheck changes it's linting based on the shebang - so if you want posix compliance, use a shebang of #!/bin/sh vs. #!/bin/bash for bash-specific linting

Some specific examples:
https://www.learnshell.org/en/Variables

#!/bin/bash
# Change this code
BIRTHDATE=
Presents=
BIRTHDAY=


# Testing code - do not change it

if [ "$BIRTHDATE" == "Jan 1, 2000" ] ; then
    echo "BIRTHDATE is correct, it is $BIRTHDATE"
else
    echo "BIRTHDATE is incorrect - please retry"
fi
if [ $Presents == 10 ] ; then
    echo "I have received $Presents presents"
else
    echo "Presents is incorrect - please retry"
fi
if [ "$BIRTHDAY" == "Saturday" ] ; then
    echo "I was born on a $BIRTHDAY"
else
    echo "BIRTHDAY is incorrect - please retry"
fi
  1. encouraging use of global variables - don't do this unless they are meant to be GLOBALS
  2. using [ instead of [[ in bash
  3. using [ instead of (( for integer comparison
  4. random casing in variables - one is capitalized, the other two are all caps? Why?
  5. randomly quoting some, but not all variables
  6. not using a main function
  7. this code doesn't even pass https://shellcheck.net/

https://www.learnshell.org/en/Loops

  1. Very odd that they did use correct bash code by managing arrays and using "${array[@]}" to loop over the array (although simple output could have been done easier in this case with something like printf "My name is %s\n" "${NAMES[@]}"), but they do something you shouldn't be encouraging anyone to do, like looping over the output of ls - this is what shell globbing is for. https://mywiki.wooledge.org/ParsingLs
  2. [ $COUNT -gt 0 ] again - this is legacy POSIX stuff - if you are using bash, this can instead be ((count > 0)) - it's odd that they use that format when 2 lines later that use arithmetic expansion in COUNT=$(($COUNT - 1)), although they use $count, when you don't use $ for variables in arithmetic expansion in bash - it is incorrect. See https://tldp.org/LDP/abs/html/arithexp.html

If you want to teach good shell scripting, getting users use to best practices, even if they don't understand them all yet, should be an important thing.

I'd recommend picking a shell standard and following it, for example https://google.github.io/styleguide/shellguide.html

@ronreiter
Copy link
Owner

Hi @Gorian. Since I'm not an expert in bash or shell in general, I'd appreciate it if you can send over some pull requests to fix these issues.

Thanks.

@Gorian
Copy link
Author

Gorian commented Dec 18, 2020

@ronreiter Sure, I'd be happy to see what I can do to help :)

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

No branches or pull requests

2 participants