Enhance the autocorrect for Style/FetchEnvVar

[j-miyake]

#10578

This PR enhances the autocorrect for Style/FetchEnvVar.

When an ENV[] is the LHS of ||, the autocorrect makes the RHS the default value of ENV.fetch.

Examples

# from
ENV['X'] || 'y'

# to
ENV.fetch('X', 'y')

# from
ENV['X'] || ENV['Y'] || a

# to
ENV.fetch('X') { ENV.fetch('Y') { a } }

# from
z || ENV['X'] || y || ENV['Z'] || a || ENV['B']

# to
z || ENV.fetch('X') { y } || ENV.fetch('Z') { a } || ENV.fetch('B', nil)

# from
ENV['X'] || y.map do |a|
  a * 2
end

# to
ENV.fetch('X') do
  y.map do |a|
    a * 2
  end
end

Please refer to the RSpec file for more examples.

---

Before submitting the PR make sure the following are checked:

• The PR relates to only one subject with a clear title and description in grammatically correct, complete sentences.
• Wrote good commit messages.
• Commit message starts with [Fix #issue-number] (if the related issue exists).
• Feature branch is up-to-date with master (if not - rebase it).
• Squashed related commits together.
• Added tests.
• Ran bundle exec rake default. It executes all tests and runs RuboCop on its own code.
• Added an entry (file) to the changelog folder named {change_type}_{change_description}.md if the new code introduces user-observable changes. See changelog entry format for details.

[bbatsov]

RSpec? :-)

[j-miyake]

Oops, thanks!

[bbatsov]

Seems to me that adds extra complexity for the end users - I generally don't see why someone would want to put basic literals in a block, as there are no performance gains for this.

[j-miyake]

Thanks for your point. If the option doesn't have much benefit for most people, I'm willing to remove it as the less code or options, the better.
Some developers don't want to have multiple ways to write code, so I thought it might be convenient for them, but it may be better we concerned about that when many people really need it :)

[bbatsov]

Indeed. I'm of the "less is more" school of thought, so I prefer to add options only when there's some demand for them.

[j-miyake]

That's true. I removed the option. 3797914

[j-miyake]

I added some additional test cases and fixed the code to make them pass.

[koic]

As I noticed in this PR, I think most ENV['TERM'] || something should be allowed. Rewriting this to ENV.fetch('TERM', something) or ENV.fetch('TERM') { something } does not have much of the following benefits:

      # This cop suggests `ENV.fetch` for the replacement of `ENV[]`.
      # `ENV[]` silently fails and returns `nil` when the environment variable is unset,
      # which may cause unexpected behaviors when the developer forgets to set it.
      # On the other hand, `ENV.fetch` raises KeyError or returns the explicitly
      # specified default value.

User can get the KeyError, but the fact that the default value is used would mean that it supplements that condition.
It also means an unexpected change for users with ENV['TERM'] || default who don't set an environment variable for ENV['TERM'] intentionally. Then KeyError would be unexpected.

[j-miyake]

Rewriting this to ENV.fetch('TERM', something) or ENV.fetch('TERM') { something } does not have much of the following benefits:

Yeah, that sounds correct. This problem somehow looks silmilar to #10582 .

[bbatsov]

As I noticed in this PR, I think most ENV['TERM'] || something should be allowed. Rewriting this to ENV.fetch('TERM', something) or ENV.fetch('TERM') { something } does not have much of the following benefits:

Still, to me this seems like the type of thing that can be optimized as the || check is essentially the same and hash has this API exactly to handle this common scenario (default value for a missing key). I do agree, however, that the optimization is not critical, so perhaps we should just have a config for the cop wether to detect and fix usages of || and or and to fix them. I, myself, would be fine if we don't and we just flag and fix all of them, as I prefer fetch + default value all of the time. Am I missing some case in which using || is actually more useful?

[j-miyake]

Rewriting this to ENV.fetch('TERM', something) or ENV.fetch('TERM') { something } does not have much of the following benefits:

User can get the KeyError, but the fact that the default value is used would mean that it supplements that condition.

Just to confirm, the above comment means that the both codes below are read as “Get 'TERM' environment variable if it's set, otherwise something", so correcting the first one doesn't make any changes to the behavior when the environment variable isn't set unexpectedly, plus, that diverges from what this cop is originally addressing, correct?

ENV['TERM'] || something

ENV.fetch('TERM', something)

[bbatsov]

It seems to me that this is what @koic meant, but he has to clarify himself. We can always tackle this separately in a different cop, although perhaps that'd be an overkill.

[koic]

the || check is essentially the same and hash has this API exactly to handle this common scenario (default value for a missing key).

I'm sorry, I was misunderstanding 💦 The above comment makes sense. So, there is no problem correcting it with compatible API.