Skip to content

more expressive implementation by using "meaningful" variable names that reflect the intent #21

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.

Sign up for GitHub

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

Closed
pvdb wants to merge 1 commit into from
Closed

more expressive implementation by using "meaningful" variable names that reflect the intent #21

Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
9 changes: 5 additions & 4 deletions lib/tmpdir.rb
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -19,10 +19,11 @@ class Dir
# Returns the operating system's temporary file path.

def self.tmpdir
['TMPDIR', 'TMP', 'TEMP', ['system temporary path', @@systmpdir], ['/tmp']*2, ['.']*2].find do |name, dir|
unless dir
next if !(dir = ENV[name]) or dir.empty?
end
candidate_dirs = ['TMPDIR', 'TMP', 'TEMP'].map(&ENV.method(:[]))

@nobu nobu Oct 25, 2022

Copy link
Copy Markdown
Member

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.

@pvdb pvdb Oct 25, 2022 •
edited
Loading

Copy link
Copy Markdown
Author

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. 😃

@nobu nobu Oct 26, 2022

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Why is name gone?

@pvdb pvdb Oct 26, 2022 •
edited
Loading

Copy link
Copy Markdown
Author

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.

@nobu nobu Oct 26, 2022

Copy link
Copy Markdown
Member

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.

@pvdb pvdb Oct 26, 2022

Copy link
Copy Markdown
Author

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! 😅

@pvdb pvdb Oct 26, 2022

Copy link
Copy Markdown
Author

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. 🤔

fallback_dirs = [@@systmpdir, '/tmp', '.']

(candidate_dirs + fallback_dirs).find do |dir|
next if !dir or dir.empty?
dir = File.expand_path(dir)
stat = File.stat(dir) rescue next
case
Expand Down