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

[Fix #10569] Fix a false positive for Style/FetchEnvVar #10603

Merged
merged 1 commit into from May 7, 2022

Conversation

koic
Copy link
Member

@koic koic commented May 6, 2022

Fixes #10569.

This PR fixes a false positive for Style/FetchEnvVar when using the same value of ENV as if condition in the body.

The ENV var as a if condition is currently allowed.
So the ENV var already used in the condition will be accepted as well.

And the following (maybe) corner case is accepted in this PR.

if ENV['foo']
  ENV.delete('foo')
  puts 'some code'
  puts ENV['foo']
  puts 'other code'
end

#10569 (comment)


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.

Fixes rubocop#10569.

This PR fixes a false positive for `Style/FetchEnvVar` when using
the same value of `ENV` as `if` condition in the body.

The `ENV` var as a `if` condition is currently allowed.
So the `ENV` var already used in the condition will be accepted as well.

And the following (maybe) corner case is accepted in this PR.

```ruby
if ENV['foo']
  ENV.delete('foo')
  puts 'some code'
  puts ENV['foo']
  puts 'other code'
end
```

rubocop#10569 (comment)
@bbatsov bbatsov merged commit 4087bae into rubocop:master May 7, 2022
@bbatsov
Copy link
Collaborator

bbatsov commented May 7, 2022

Thanks!

@koic koic deleted the fix_a_false_for_style_fetch_env_var branch May 7, 2022 06:57
@inkstak
Copy link

inkstak commented May 19, 2022

Here is another false positive :

if condition || ENV["FOO"]
   puts ENV.fetch("FOO")
end

# if condition || ENV["FOO"]
#                 ^^^^^^^^^

while inversing conditions is accepted by rubocop:

rubocop -V
1.29.1 (using Parser 3.1.2.0, rubocop-ast 1.18.0, running on ruby 2.7.6 x86_64-darwin21)
  - rubocop-performance 1.13.3
  - rubocop-rails 2.14.2
  - rubocop-rspec 2.11.1

@AlexWayfer
Copy link
Contributor

@inkstak I think, the original issue was about fetch suggestions inside if-block. So, if you'd get an offense about puts line — that would be this issue.

In your case, it's OK to use (I guess):

if condition || ENV.fetch('FOO', false)
  puts ENV['FOO']
end

But I'd still recommend to use fetch inside if because you can get into there by condition, without confidence that there is ENV['FOO'] (until it's || and not &&).

@inkstak
Copy link

inkstak commented May 19, 2022

Yes, that may be another issue : the false positive is in the if statement and not the if block.

# if condition || ENV["FOO"]
#                 ^^^^^^^^^

It's definitely because the condition might be truthy I used the fetch method in puts ENV.fetch("FOO")

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 this pull request may close these issues.

Style/FetchEnvVar check for if condition
4 participants