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

Style/FetchEnvVar autocorrect not very readable in boolean-or chains #10788

Closed
merrilymeredith opened this issue Jul 5, 2022 · 3 comments · Fixed by #10819
Closed

Style/FetchEnvVar autocorrect not very readable in boolean-or chains #10788

merrilymeredith opened this issue Jul 5, 2022 · 3 comments · Fixed by #10819

Comments

@merrilymeredith
Copy link

The Style/FetchEnvVar cop is autocorrecting to code that doesn't seem like a style improvement when given code that accepts a few possible ENV variable names or has a chain of alternatives. Note that since ENV values are strings, || does not risk discarding a 0 in the environment as it's a truthy "0" right now.

-      ENV["LOG_NAME"] || Core.app_name || File.basename($PROGRAM_NAME)
+      ENV.fetch("LOG_NAME") { Core.app_name } || File.basename($PROGRAM_NAME)
-      ENV["BUILD_METADATA"] || ENV["APPBASE_BUILD_METADATA"] || File.read("/BUILD_METADATA")
+      ENV.fetch("BUILD_METADATA") { ENV.fetch("APPBASE_BUILD_METADATA") { File.read("/BUILD_METADATA") } }

This satisfies the cop and might be a better autocorrect when there's more than a single alternative, but it's still a bit odd:

ENV.fetch("FOO", nil) || ENV.fetch("BAR", nil) || baz

and short sets of options seem like too little to switch to an array-oriented way of doing this:

ENV.values_at("BUILD_METADATA", "APPBASE_BUILD_METADATA").compact.first || File.read("/BUILD_METADATA")

I'd ask someone to prefer simplicity over cleverness if I saw the above.

One could definitely get out of hand with with a boolean chain but I'm sure they'd cross other cops in doing so, while the longest examples I'm dealing with here are 3 terms. Four is the point where I'd personally switch to a different style or refactor.

I think this cop could be relaxed when it finds ENV[...] || ... on the LHS of another || ...?

RuboCop version

1.30.1 (using Parser 3.1.2.0, rubocop-ast 1.18.0, running on ruby 3.1.2 x86_64-linux)
  - rubocop-rspec 2.11.1
@jonas054
Copy link
Collaborator

@j-miyake Do you have any comments on this?

@j-miyake
Copy link
Contributor

j-miyake commented Jul 13, 2022

Indeed. The autocorrect of this cop may decrease readability like the case reported by this issue. Initially, I took this as a trade-off, but considering some improvements or comments (like #10652, #10585 (comment)) made until today, ENV[] in LHS of || operator probably should be allowed because the || is put by the developer who knows the LHS may be nil. I would like to hear others' thoughts.
(cc @koic @bbatsov)

@j-miyake
Copy link
Contributor

The code would be like this if we relax the cop.

j-miyake added a commit to j-miyake/rubocop that referenced this issue Jul 18, 2022
… of `||`

Fixes rubocop#10788

This PR relaxes `Style/FetchEnvVar` cop to allow `ENV[]` in LHS of `||`.
`ENV[]` in LHS of `||` operator can be allowed by this cop because
the `||` is put by the developer who knows the LHS may be `nil`.
bbatsov pushed a commit that referenced this issue Jul 21, 2022
Fixes #10788

This PR relaxes `Style/FetchEnvVar` cop to allow `ENV[]` in LHS of `||`.
`ENV[]` in LHS of `||` operator can be allowed by this cop because
the `||` is put by the developer who knows the LHS may be `nil`.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging a pull request may close this issue.

3 participants