Closed jasonkarns closed 8 years ago
The --usage
addition makes sense, and the replacement of sed
is a nice touch. I'm not sure about the rest.
exec
means one less process running. Does it hurt anything?*
to ./*
doesn't matter because echo_lines
doesn't accept options. Plus it muddies up the echo with ./
-a
, but I don't think they apply when running under bash.Most the changes (except exec
), were caught with the shellcheck
tool.
exec
because it seemed inconsistent with other invocations. The only rationale I have against it is that it makes the exit implicit. Indeed, this PR itself is broken because I forgot to add exit
s when I removed exec
:/ . And the exit status is carried from the exec'd command; which may or may not be desired. I'm assuming it's not desired in this case, because it's only emitting help usage. (An error while emitting help usage probably shouldn't cascade?) I'll update the PR per your direction, but I would suggest it be a bit more consistent with exec
usage (using it in more places where early exit is desired).echo_lines
doesn't accept options, but that doesn't mean that whatever it may be piped to (in the future) does not. echo_lines_without_symlinks
doesn't accept options, either, but its output is filtered (unmodified) and immediately piped to sed
, which does take options.-a
for stylistic reasons in addition to portability concerns, but that's subjective, naturally. Up to you.exec
and add an exit 1
afterwards (which means expanding the ||
case to a proper if
).sed
doesn't read arguments from stdin so I don't understand your concern about piping here.If help is invoked due to invalid arguments, the exit status should be a failure, actually. So we should drop the exec and add an exit 1 afterwards (which means expanding the || case to a proper if).
good call
sed doesn't read arguments from stdin so I don't understand your concern about piping here.
yep, brain fart
I don't think stylistic preference is reason enough to change existing, functioning code in either of these cases.
legit
Changes forthcoming
unalias
. Unalias help was actually printing alias help. Was this intentional? Assuming that unalias should be printing its own help/usage output, considering it has help/usage doc at top of file.also created and refactored to a separate fail helper function to handle printing an error message to stderr and exiting with 1. pushed in separate branch. can merge to this pr if desired or open separate pr.
https://github.com/jasonkarns/rbenv-aliases/compare/help-usage...jasonkarns:fail-helper
Please clean up the history. One commit would be fine or you can try to split it into a small number.
Do you want the fail-helper refactor included or no?
Uh I guess leave it out for now.
squashed to 3 commits
shellcheck fixes
fixed some things that shellcheck static analysis found
only print --usage help, not entire help output when given unknown args
remove extraneous 'exec'
Tests are failing on master and failing for identical reasons on this branch.