On Monday 14 January 2013 22:20:20 Duncan wrote:
> Mike Frysinger posted on Mon, 14 Jan 2013 17:09:51 -0500 as excerpted:
> >>> + [[ ${BUILD_DIR} ]] || die "${FUNCNAME}: BUILD_DIR not set."
> >>
> >> really should use -n there
> >>
> >> Doesn't matter.
> >
> > the point wasn't "will it work". it's more "how easy is it to glance at
> > code and know what it is doing".
>
> Indeed. But arguably standalone [[ ${var} ]] tests ARE easier to "know
> what it doing."
>
> 1) The [[ $var ]] case is exactly one of one. There's no misinterpreting
> it. Even if you don't know the rule by rote, given that it's a boolean
> test on a string, there's logically only one way to parse it. No
> mistakes to make.
not really. when you see [[ ${var} ]], which did the dev actually mean to do:
if ( ${var} ) ; then
if ${var} ; then
if [[ -n ${var} ]] ; then
if i see the -n, i'm pretty confident they meant to do a string test. if i
don't, then i need to read the rest of the code to figure out the meaning & use
of ${var} to see if they screwed up.
and yes, this still happens. i've seen fixes in the last month along these
lines.
> 2) By contrast, -n is only one of a whole list of -X style tests, and one
> must stop and think, "Let's see... Oh, yes, -n was non-null."
i don't buy that. -z and -n are the most common `test` tests out there. if
you can't handle that, then you probably can't handle a lot of the
ebuilds/eclasses.
> 4) You are arguing the "at a glance" position, but didn't even MENTION
> the needlessly negated logic of [[ -n $string ]] || ... , which could be
> rewritten to avoid the || negation as [[ -z $string ]] && ...
that depends on the code. there is no correct answer here.
> 5) [[ ]] is already a bashism while the standalone string test is common
> shell. Surely you're not arguing that people familiar enough with the
> [[ ]] || construct to parse it at a glance can't equally capably parse
> the a standalone string test, given its use in non-bash shell context as
> well.
bashisms are irrelevant. we already stated the whole tree is bash.
-mike
signature.asc
Description: This is a digitally signed message part.
