WIP: ENV access clean ups

[richardkmichael]

Summary

• Change ENV.fetch(k) { "fish" } to ENV.fetch(k, "fish")

There are numerous calls to ENV#fetch() which use a single expression block to specify the return value if the key is missing. Block creation is unnecessary in this situation, because the return value can be given to fetch() directly. In my opinion, specifying the default value directly is a simpler use of the Hash-like ENV API. In the YAML files immediately encountered by a Rails developer, an opportunity is presented to teach the API for this common use-case (i.e., get key a, else b). There are occurrences of #fetch(a, 'other') elsewhere in Rails code, so the change here also makes Rails code more consistent.

• Change ENV[k] || "fish" to ENV.fetch(k, "fish") (leave ENV[k] || "fish" || "other" alone)

In code where an ENV key accessed and || is used for a value if it missing, prefer ENV#fetch() with a default value. Obviously ENV[k] || "fish" is not equivalent to ENV.fetch(k, "fish") when ENV[k] == false, but ENV values are usually (always?) string and so in practice they are equivalent, as no Ruby string is falsy. However, in an "or-chain" (i.e., a || b || c) do not adjust the first two terms, because intent is clearer and the value of b makes this usage more complicated. Let's wait to see what the Rails test-suite reports.

• Micro-cleanups while code reading

Other Information

Ran rubocop locally, with no violations. Waiting on the Rails testsuite via the PR.

[hahmed]

Here are the benchmarks: (credit: https://sikac.hu/fetch-vs-bracket-or-for-default-value-in-ruby-9bfd3b4cc37e)

>> RUBY_VERSION
=> "2.6.5"
>> require "benchmark/ips"
=> false
>> Benchmark.ips do |x|
>> x.report("fetch_block") {{}.fetch(:non_existance) { true } }
>> x.report("fetch") {{}.fetch(:non_existance, true)}
>> x.report("[] ||") { {}[:non_existance] || true }
>> end; nil
Warming up --------------------------------------
         fetch_block   658.825k i/100ms
               fetch   992.967k i/100ms
               [] ||     1.068M i/100ms
Calculating -------------------------------------
         fetch_block      6.729M (± 0.8%) i/s -     34.259M in   5.091536s
               fetch      9.987M (± 0.9%) i/s -     50.641M in   5.071062s
               [] ||     10.721M (± 0.5%) i/s -     54.467M in   5.080644s

Ideally we should be using: hash[:key] || "value"

[jonathanhefner]

I believe this counts as a cosmetic change, but if this is a style we want to enforce, we can upgrade to RuboCop v0.86 and activate the Style/RedundantFetchBlock cop (see rubocop/rubocop#8147).

(Note that an upgrade to RuboCop v0.85 is currently in progress: #39527.)

[richardkmichael]

@hahmed Thank you for pointing me to the benchmarks. Clearly, we want to avoid fetch with block. :-) The 10% between "fetch with default value" and "[] || default" is a shame. Since ENV is Ruby core and serves exactly the purpose of accessing the environment, I think a re-factoring or object surface argument probably doesn't apply, so maybe ENV[:k] would be ok for maximum performance. (Concerning Hash#fetch(), I might give up the 10% for those reasons, depending on the hotness of the code-path.) Being selective is why I left the commits separate for now.

@jonathanhefner Thank you for mentioning the recent cop. I tried it just now, and unfortunately it has a bug with empty blocks (e.g., {}.fetch(:a) { }) causes an error, and similar code occurs in Rails). I have opened rubocop/rubocop#8227, when it's fixed perhaps enabling the cop would be the way to go. Cheers!

Rubocop (>0.87) will work with empty blocks. However, not all receivers of #fetch() are instances of Hash, so Rubocop is occasionally wrong to suggest fetch(10) { 20 } should be changed to fetch(10, 20). Most Rails objects which implement #fetch() may be compatible with Hash#fetch(), but not always. In particular, ActiveRecord::Type::TypeMap#fetch() and ActiveSupport::Cache#fetch() are not. The latter, as Rails.cache in Rails projects, but not Rails itself, is handled by Rubocop and discussed here and here.

[rails-bot]

This pull request has been automatically marked as stale because it has not had recent activity. It will be closed if no further activity occurs.
Thank you for your contributions.