Skip to content
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

refactor: simplify Rake::Application#system_dir method #591

Merged
merged 1 commit into from
Dec 9, 2024

Conversation

pvdb
Copy link
Contributor

@pvdb pvdb commented Oct 2, 2024

Due to it being a tad convoluted, it took me longer than it probably should have to grok what the Rake::Application#system_dir method was trying to do.

I think this simplification makes it a lot more obvious, especially as the same ...

hash[:lookup_key] || :default_value

... idiom is already used elsewhere in the rake codebase.

It also avoids a double hash lookup: one lookup to see if RAKE_SYSTEM is set in the ENV hash, and - if it is - exactly the same lookup again to actually fetch its value. 🎉

PS - I was tempted to also include the following change in this PR ... happy to take your steer @hsbt : include it in this one, or issue a separate PR instead? (or else forget about it altogether if it's totally bonkers?) 😅

--- a/lib/rake/application.rb
+++ b/lib/rake/application.rb
@@ -765,7 +765,7 @@ module Rake
       end
     else
       def standard_system_dir #:nodoc:
-        File.join(File.expand_path("~"), ".rake")
+        File.join(Dir.home, ".rake")
       end
     end
     private :standard_system_dir

@pvdb pvdb changed the title avoid double ENV["RAKE_SYSTEM"] lookup chore: simplify Rake::Application#system_dir method Oct 2, 2024
@hemalvarambhia
Copy link

👍

@pvdb pvdb changed the title chore: simplify Rake::Application#system_dir method refactor: simplify Rake::Application#system_dir method Dec 8, 2024
@hsbt hsbt merged commit 5625c0d into ruby:master Dec 9, 2024
28 checks passed
@pvdb pvdb deleted the simplify_rake_system_dir branch December 9, 2024 10:45
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Development

Successfully merging this pull request may close these issues.

3 participants