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