p8952 / bocker

Docker implemented in around 100 lines of bash
https://www.p8952.info/
GNU General Public License v3.0
11.26k stars 715 forks source link

Shorter case statement #4

Closed TomiBelan closed 9 years ago

TomiBelan commented 9 years ago

Just for fun: By using bocker_$1 "${@:2}", you can dynamically call different bash functions. This makes the case statement much shorter, saving 9 lines. (Unfortunately, this is incompatible with #1.)

Note: I confess that I barely tested this. Please make sure I didn't break anything (especially run).

p8952 commented 9 years ago

Looks good to me.

Just a couple of issues:

If you can update the PR with those changes I'd be happy to merge it.

SkUrRiEr commented 9 years ago

Just so you know, the actual functionality isn't incompatible, but the implementation is.

The help comments can literally be anywhere in the script, they don't have to be with the case statements, I just put them there as that's where the actual "names" of the commands are defined.

SkUrRiEr commented 9 years ago

Tomi,

I've rebased your commit over mine with the changes I mentioned in my previous comment and added that to a new branch in my repo called "for_tomi". See this commit: fbddd63e1bef86ba3d43394e7fc96281bd4f4730.

I'm also not sure if you broke the run command or not. Either way, I've made another commit on that branch: e78cdcd24fd1b6d9563c285cb1c6b66c8f11975e that saves another line and might make things work better.

Essentially the difference is using $* instead of $@: "$@" translates into "a" "b" "c" "$*" translates into "a b c" As ${*:2} is also valid, I've removed your "cmd" variable and just used ${*:2} everywhere in the run command.

(Edited to use a commit with a correct description and fix formatting.)

SkUrRiEr commented 9 years ago

And now there's commits in that branch to handle @p8952's comments. Feel free to squash as appropriate.

TomiBelan commented 9 years ago

I am a bit surprised that ${*:2} works, since the bash manual explicitly mentions the rule for ${@:2} but ${*:2} is undocumented. Going by the documentation, ${*:2} should be a substring of $*. But apparently not. Personally I still like cmd=${@:2} more, but it's your call.

http://www.gnu.org/software/bash/manual/bash.html#Shell-Parameter-Expansion

${parameter:offset} ${parameter:offset:length} This is referred to as Substring Expansion. It expands to up to length characters of the value of parameter starting at the character specified by offset. If parameter is ‘@’, an indexed array subscripted by ‘@’ or ‘*’, or an associative array name, the results differ as described below. [...] If parameter is ‘@’, the result is length positional parameters beginning at offset.

I'll amend my commit, so that it can be merged separately from #1 if desired.

SkUrRiEr commented 9 years ago

Testing a shell script containing the following:

echo "TEST $@ TEST ${@:2}"
echo "TEST $* TEST ${*:2}"

proves that both variants work identically.

${@:2} is documented, ${*:2} isn't, so I say use the former.

p8952 commented 9 years ago

Fair point @SkUrRiEr, not sure where I got the idea about using ${*:2} from then. Let's do it that way.

TomiBelan commented 9 years ago

OK, I changed it back to cmd=${@:2}.

p8952 commented 9 years ago

Perfect! Sorry for making you go back and forth with that.