On Thu, 15 Feb 2018 15:11:13 -0500, Dave Reisner wrote: > > On Thu, Feb 15, 2018 at 02:49:45PM -0500, Luke Shumaker wrote: > > - [[ -f ${FTP_BASE}/${PKGPOOL}/${pkgname}-${pkgver}-${pkgarch}${PKGEXT} > > ]] && return 1 > > - [[ -f > > ${FTP_BASE}/${PKGPOOL}/${pkgname}-${pkgver}-${pkgarch}${PKGEXT}.sig ]] && > > return 1 > > + [ -f "${FTP_BASE}/${PKGPOOL}/${pkgname}-${pkgver}-${pkgarch}"${PKGEXT} > > ] && return 1 > > + [ -f > > "${FTP_BASE}/${PKGPOOL}/${pkgname}-${pkgver}-${pkgarch}"${PKGEXT}.sig ] && > > return 1 > > Rather than making this stand out like a sore thumb for the next person > to trip over, why don't we just define a "file_exists" function? > > file_exists() { > [[ -f $1 ]] > } > > Now you're free to do this: > > file_exists > "${FTP_BASE}/${PKGPOOL}/${pkgname}-${pkgver}-${pkgarch}"${PKGEXT} && return 1 > > Expansion is taken care of by the shell before you pass to exists, and > the check does what you'd expect.
That's not a bad idea. But then someone reading the code might wonder "why does such a trivial function exist?". I think it would be silly, and ultimately hurt readability to go through and replace all "[[ -f ... ]]" instances with "file_exists", and if we don't do that, then there's a weird magic question of "when should I use [[ -f ]] and when should I used file_exists?" Ultimately, this way doesn't hide anything, and has everything the next person needs to know right there. Sure, they'll have to be careful not to trip. The next patch I sent changes it to PKGEXT_glob, to make it stand out a little more. I'm also working on a `make lint`/shellcheck patchset that will add `# shellcheck disable=SC2086` directives to each line to avoid shellcheck complaining about PKGEXT_glob being unquoted, drawing even more attention to it, to avoid tripping. -- Happy hacking, ~ Luke Shumaker