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

Cop idea: Set the default value of ENV.fetch by autocorrect #10578

Closed
j-miyake opened this issue Apr 24, 2022 · 4 comments
Closed

Cop idea: Set the default value of ENV.fetch by autocorrect #10578

j-miyake opened this issue Apr 24, 2022 · 4 comments

Comments

@j-miyake
Copy link
Contributor

j-miyake commented Apr 24, 2022

Is your feature request related to a problem? Please describe.

From: #10502 (comment)

With Style/FetchEnvVar, code like

value = ENV['EXAMPLE_KEY'] || 'default_value'

gets autocorrected to

value = ENV.fetch('EXAMPLE_KEY', nil) || 'default_value'

Autocorrecting like below helps people reduce their manual changings like above.

value = ENV.fetch('EXAMPLE_KEY', 'default_value')

# This version would be better on performance if the default value is calculated by an expensive computation.
value = ENV.fetch('EXAMPLE_KEY'){'default_value'}

Describe the solution you'd like

  • When ENV[] is the left-hand of ||, the autocorrect can move the right-hand of || to the default value of ENV.fetch.
  • In some cases, the right hand of || can't be the default value (e.g., next), so we need to carefully handle them.
@j-miyake j-miyake changed the title Cop idea: Style/FetchEnvVar - Set the default value of ENV.fetch Cop idea: Set the default value of ENV.fetch by autocorrection Apr 24, 2022
@bbatsov
Copy link
Collaborator

bbatsov commented Apr 25, 2022

Seems like a good idea to me.

As for the auto-correct - I guess if the default is some literal or a local value we can use the first form and if it's a method call we can go for the block style, although I don't have a strong preference. The performance gains would be notable only if the computation of the default value is expensive.

@j-miyake j-miyake changed the title Cop idea: Set the default value of ENV.fetch by autocorrection Cop idea: Set the default value of ENV.fetch by autocorrect Apr 25, 2022
@ShockwaveNN
Copy link
Contributor

Please note that if you got code like this:

return ENV['foo'] if ENV.key?('foo')

File.read('file_if_foo_not_exists')

Code like this will work fine if ENV['foo'] exists and File file_if_foo_not_exists not exists

But if you by similarity with proposal from this issue change it to

ENV.fetch('foo', File.read('file_if_foo_not_exists'))

If will fail in any case if file_if_foo_not_exists do not exists (even if ENV['foo'] is initialized)

@j-miyake
Copy link
Contributor Author

Great point. That would be one of the cases we should use the block style.

ENV.fetch('foo') { File.read('file_if_foo_not_exists') }

In my opinion, in most cases always using the block style is safer and reduces the rules. Any thoughts?

@j-miyake
Copy link
Contributor Author

j-miyake commented Apr 28, 2022

I created a PR for this.
#10585
I made the default value style configurable so that users can choose their preferred way.
Any help in finding missing test cases or edge cases is very appreciated.

@j-miyake j-miyake closed this as completed May 2, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

No branches or pull requests

3 participants