todotxt / todo.txt-cli

☑️ A simple and extensible shell script for managing your todo.txt file.
http://todotxt.org
GNU General Public License v3.0
5.56k stars 713 forks source link

use builtin "command -v" instead of which #308

Closed wasnio closed 3 years ago

wasnio commented 4 years ago

We shouldn't use which because it is not built-in.

karbassi commented 4 years ago

I agree.

@inkarkat thoughts?

inkarkat commented 4 years ago

The program also uses grep, sed and awk, which also aren't built-in (but arguably much harder to implement in Bash). Of course, fewer dependencies are always desirable, but is there actually a distribution where which isn't available by default?

Unfortunately, the command -v replacement isn't 100% compatible; it accepts any callable executable, function, and even aliases. But as $PAGER is invoked inside double quotes (and it has to, as the environment variable may contain a filespec with special characters), aliases don't expand there. So if one mistakenly sets PAGER to an alias, the program would attempt to invoke it. (On the other hand, currently one could assign a function to PAGER and it would be ignored, which also is not fully correct.)

rico-chet commented 4 years ago

I remember stumbling upon situations where which wasn't available, but I guess this was on embedded targets or some stripped-down containers only.

The command -v replacement is superior, in my opinion, since it allows for more flexibility, e.g. by using exported functions which can be handy when creating test doubles. Aliases and functions aren't inherited per default, so if anyone wants to use a function in PAGER, she will need to export it and failing to do so is a mistake anyway.