Hi Simon, Simon Tournier <zimon.touto...@gmail.com> writes:
> Hi Maxim, > > On Mon, 16 Dec 2024 at 11:53, Maxim Cournoyer <maxim.courno...@gmail.com> > wrote: > >>> +_info() >>> +{ >>> + if [ "$(type -P info)" ]; then >>> + _msg "$1" >>> + else >>> + _msg "${WAR}Please install Info reader; see package 'info-reader'" >>> + _msg "$1" >>> + fi >>> +} >> >> It seems odd to me to "overload" _msg into _info that deals with some side >> effect; I'd rather see this conditional explicit at the message printing >> site. > > It was to avoid the duplication of the exact same conditional with the > exact same message. > > I do not have an opinion… Hm. I agree duplication is not nice. Probably a naming issue ;-) >> Also, your test is testing for the empty string when info is not found, >> not the exist status, which is wrong. > > Please note that the script already uses: > > if [ "$(type -P pidof)" ]; then > if [ ! "$(pidof nscd)" ]; then > > And I have only respected the same. :-) According to git blame these lines were also authored by you 4 years ago, ha! >> not the exist status, which is wrong. I think you meant something like: >> >> --8<---------------cut here---------------start------------->8--- >> if type -P info >/dev/null then [...]; fi >> --8<---------------cut here---------------end--------------->8--- > > Well, I am not a Bash expert but I guess that’s the same result in > practise, no? It checks the exit status instead of the captured string output. While it's not that bad in that case, in general I find checking for the exit status a much more reliable and clean option. > Both $() and "" used in tandem makes the test sound, from my > understanding. Hm. Is [ "something" ] true and [ "" ] false? Apparently it is, but I'd argue that's not very clear, especially when there are explicit test operations to check for an non-empty or empty string (test -n and test -z). >> But this got me curious again... could we instead automate the >> installation of info post-installation? > > It appears to me unrelated to this change at hand. :-) It's related in that if the user opted to install 'info-reader' (on by default), we wouldn't have to warn anything about, but yes, we can do so later if you prefer, as I expect it's not that trivial. I don't have strong feelings about the change as-is anymore, but I may refactor the type -P checks to use the alternative style outlined above, if you don't mind. -- Thanks, Maxim