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 check for if condition #10569

Closed
ShockwaveNN opened this issue Apr 22, 2022 · 2 comments · Fixed by #10603
Closed

Style/FetchEnvVar check for if condition #10569

ShockwaveNN opened this issue Apr 22, 2022 · 2 comments · Fixed by #10603

Comments

@ShockwaveNN
Copy link
Contributor

@ShockwaveNN ShockwaveNN commented Apr 22, 2022

Not sure that rubocop can check it or dynamic check is required for such checks, but I'll report it anyway

Steps to reproduce the problem

  1. Create test.rb with content:
# frozen_string_literal: true

if ENV['foo']
  puts 'some code'
  puts ENV['foo']
  puts 'other code'
end
  1. run rubocop -A check
    result code will be:
# frozen_string_literal: true
    
if ENV['foo']
  puts 'some code'
  puts ENV.fetch('foo', nil)
  puts 'other code'
end

So fetch is literally useless in code like this

RuboCop version

$ [bundle exec] rubocop -V
1.28.1 (using Parser 3.1.2.0, rubocop-ast 1.17.0, running on ruby 3.1.2 x86_64-linux)
  - rubocop-performance 1.13.3
@ShockwaveNN
Copy link
Contributor Author

@ShockwaveNN ShockwaveNN commented Apr 22, 2022

But in same time there may be code like this:

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

And fetch is required in this code, so I'm thinking that correct check via rubocop in cases like this cannot be done

@rthornton128
Copy link

@rthornton128 rthornton128 commented May 5, 2022

ENV['foo'], ENV.fetch('foo', nil) and ENV.dig('foo') are all functionally equivalent. All three return nil when ENV does not contain the matching key. Perhaps what this cop should state, when recommending ENV.fetch('foo', nil) as an alternative, is the explicitness of returning nil?

Ultimately, a developer/team/project is free to ignore or disable cops. We should not blindly use all of them. If we think the cop has no value or does not apply to our code base, we should disable them. I think that applies to this case.

EDIT: Misread the issue.

koic added a commit to koic/rubocop that referenced this issue May 6, 2022
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 currentry 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)
koic added a commit to koic/rubocop that referenced this issue May 6, 2022
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 pushed a commit that referenced this issue May 7, 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.

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

#10569 (comment)
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.

2 participants