more expressive implementation by using "meaningful" variable names that reflect the intent#21
more expressive implementation by using "meaningful" variable names that reflect the intent#21pvdb wants to merge 1 commit intoruby:masterfrom
Conversation
| unless dir | ||
| next if !(dir = ENV[name]) or dir.empty? | ||
| end | ||
| candidate_dirs = ['TMPDIR', 'TMP', 'TEMP'].map(&ENV.method(:[])) |
There was a problem hiding this comment.
This always looks up 3 environment variables, even unused.
There was a problem hiding this comment.
True, but the original implementation always creates 3 - arguably quite convoluted - arrays, even unused.
['system temporary path', @@systmpdir], ['/tmp']*2, ['.']*2](especially "wasteful" IMO considering that most of the time the tmpdir will be set in the process' environment and these 3 fallbacks don't even come into play) 😅
Like I said, it's a style-change more than a functional one, but I quite like the expressiveness personally. 😃
There was a problem hiding this comment.
It's no longer needed: it's now iterating over a list of 6 directory paths, whereas previously it was iterating over a combination of 3 environment variable names (TMPDIR, TMP and TEMP, which got turned into a directory path inside the block) and 3 directory paths (@@systmpdir, /tmp, and .)
For the first three entries, the dir in (name, dir) was always empty, as it was determined inside the block
For the last three entries, the name in (name, dir) was superfluous and totally unused.
This new version avoids all that confusion, and doesn't need to create the 3 superfluous, convoluted arrays I mentioned before: it's now iterating over a homogeneous collection of just 6 actual directory paths.
There was a problem hiding this comment.
It is still needed for the warning messages.
There was a problem hiding this comment.
Argh, I see what you mean... I (wrongly!) assumed, because all tests passed, it was a kosher refactor, which clearly it isn't! 🤦
Back to the drawing board! 😅
There was a problem hiding this comment.
Oh, IC... the various assert_warn() assertions in the tests only check part of the warning message, not the entire thing. 🤔
2 participants
Thanks for accepting #16, @nobu, and for fixing #17 ... much appreciated! 😊
Here's another quick refactor for your consideration... not a functional change, just a style change, but hopefully you like!
Note that I considered making both
candidate_dirsandfallback_dirsclass variables - similar to@@systmpdir- but that would require a very significant refactor of the unit tests, so I've not done that in this PR.