-
Notifications
You must be signed in to change notification settings - Fork 11
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
more expressive implementation by using "meaningful" variable names that reflect the intent #21
base: master
Are you sure you want to change the base?
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This always looks up 3 environment variables, even unused.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Why is name
gone?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It is still needed for the warning messages.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Oh, IC... the various assert_warn()
assertions in the tests only check part of the warning message, not the entire thing. 🤔
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_dirs
andfallback_dirs
class 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.