On Thu, 15 Feb 2018 16:57:26 -0500, Eli Schwartz wrote:
On 02/15/2018 04:43 PM, Luke Shumaker wrote:
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?"
Why would it hurt readability?
Why won't people be able to read the comments I wrote documenting the function in my working tree?
Every Bash programmer knows what [[ -f ]] does. Sure - "file_exists" is self-explanatory, and - a comment there might better explain why it exists instead of using [[ -f ]] But then it's another silly little thing that they have to keep in mind everywhere in the codebase. What about just adding a comment: # We can't use [[ ]] for these 2 checks because glob expansion # needs to be performed on $PKGEXT. It avoids someone tripping over it in the future, and avoids adding another unnecessary abstraction to the codebase.
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.
I want to make dbscripts more readable, not less. Just reverting every new thing when *both* are broken, is not something I want to do.
Huh? My version isn't broken. Unless you mean that the glob is more restrictive than the one used by makepkg? -- Happy hacking, ~ Luke Shumaker