search

exercism / v2-configlet

Tool to assist in managing Exercism language tracks.
MIT License
16 stars 23 forks source link

fetch-configlet: style refactoring #167

Closed guygastineau closed 5 years ago

guygastineau commented 5 years ago
  1. Reduced unnecessary command substitution wrappers around case statements for variable assignment. It is safer this way.
  2. Used appropriate quoting everywhere. This wasn't necessary everywhere, but the consistency is nice (and it doesn't hurt anything.
  3. moving the assignment pipe-chain for VERSION into a function makes the actual substitution assignment to VERSION much cleaner.
  4. Good rules for case include:
    • single line cases must have 1 space to either side (between pattern and ;;). I aligned them for readability too ;)
    • multi-line cases start indented on next line and ;; comes on its own line. (some people argue that it should be at the same indent level as the patterns, but I think it looks much nicer the way it is now ;) )
  5. Finally, renamed LATEST to RELEASES (just one level higher in the path hierarchy), so it could be reused to save line space. Now all lines are under 80 characters :tada:

EDIT: I also added -e to the set options at the head of the script. This will make it exit on any error without continuing. Hopefully it helps make things clearer when Travis fails.

guygastineau commented 5 years ago

PS. I noticed the CI pipeline here doesn't test this script. I verified this script worked from my Arch Linux desktop using Bash v5.0.7(1)release. It should work with any version 3.x or higher, but this is not confirmed. If anyone wants to try this on a mac with the default shell I would encourage that before merging ;) Maybe test with bash on windows too.

Side Note: I usually right align the patterns in a case block, but not everybody likes that...

kytrinyx commented 5 years ago

I'll test this on my mac now. I don't have access to windows.

kytrinyx commented 5 years ago

It fails with the same error both on master and here:

tar: could not chdir to 'bin/'

In other words, this doesn't break anything that isn't already broken. Merging!

guygastineau commented 5 years ago

Interesting... Has this always failed on Mac? We're you running it from the scripts/ directory?

kytrinyx commented 5 years ago

I just downloaded the script to my desktop and ran it with bash fetch-configlet.

It almost certainly didn't fail on the mac a few years ago when I wrote the first version (maybe?) but it's been ages and many many mac upgrades since.

guygastineau commented 5 years ago

Strange. I assume you have a bin/ in you home directory.

I would normally wonder if the tar that ships with Mac is not gnu, but the error shows that the tar executable knows what is being asked of it...

kytrinyx commented 5 years ago

I have a ~/bin and a /bin.