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

Add new Style/FetchEnvVar cop #10502

Merged
merged 13 commits into from
Apr 13, 2022
9 changes: 9 additions & 0 deletions .rubocop.yml
Original file line number Diff line number Diff line change
Expand Up @@ -42,6 +42,15 @@ Style/IpAddresses:
Exclude:
- spec/rubocop/cop/style/ip_addresses_spec.rb

Style/FetchEnvVar:
AllowedVars:
- HOME
- RUBOCOP_CACHE_ROOT
- XDG_CACHE_HOME
- XDG_CONFIG_HOME
- CIRCLE_STAGE
- CI
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

How were these 6 variable names chosen as exceptions to the new rule?

Copy link
Contributor Author

@j-miyake j-miyake Apr 11, 2022

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks for pointing it out. They are the environment variables used in this project. I listed them here to get opinions on how these should be. I think we can replace almost all ENV[...] in this project with ENV.fetch(..., nil), except that here probably should be replaced with ENV.fetch(...) without the default values. Do you have any opinions?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I added a commit to show how the code will be.
1901117


Layout/EndOfLine:
EnforcedStyle: lf

Expand Down
1 change: 1 addition & 0 deletions changelog/new_add_new_fetch_env_var.md
Original file line number Diff line number Diff line change
@@ -0,0 +1 @@
* [#10502](https://github.com/rubocop/rubocop/pull/10502): Add new `Style/FetchEnvVar` cop. ([@johnny-miyake][])
10 changes: 10 additions & 0 deletions config/default.yml
Original file line number Diff line number Diff line change
Expand Up @@ -3515,6 +3515,16 @@ Style/ExponentialNotation:
- engineering
- integral

Style/FetchEnvVar:
Description: >-
This cop suggests `ENV.fetch` for the replacement of `ENV[]`.
Reference:
- https://rubystyle.guide/#hash-fetch-defaults
Enabled: pending
VersionAdded: '<<next>>'
# Environment variables to be excluded from the inspection.
AllowedVars: []

Style/FileRead:
Description: 'Favor `File.(bin)read` convenience methods.'
StyleGuide: '#file-read'
Expand Down
1 change: 1 addition & 0 deletions lib/rubocop.rb
Original file line number Diff line number Diff line change
Expand Up @@ -484,6 +484,7 @@
require_relative 'rubocop/cop/style/expand_path_arguments'
require_relative 'rubocop/cop/style/explicit_block_argument'
require_relative 'rubocop/cop/style/exponential_notation'
require_relative 'rubocop/cop/style/fetch_env_var'
require_relative 'rubocop/cop/style/file_read'
require_relative 'rubocop/cop/style/file_write'
require_relative 'rubocop/cop/style/float_division'
Expand Down
113 changes: 113 additions & 0 deletions lib/rubocop/cop/style/fetch_env_var.rb
Original file line number Diff line number Diff line change
@@ -0,0 +1,113 @@
# frozen_string_literal: true

module RuboCop
module Cop
module Style
# 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.
#
# @example
# # bad
# ENV['X']
# ENV['X'] || z
# x = ENV['X']
#
# # good
# ENV.fetch('X')
# ENV.fetch('X', nil) || z
# x = ENV.fetch('X')
#
# # also good
# !ENV['X']
# ENV['X'].some_method # (e.g. `.nil?`)
#
class FetchEnvVar < Base
extend AutoCorrector

MSG = 'Use `ENV.fetch(%<key>s)` or `ENV.fetch(%<key>s, nil)` instead of `ENV[%<key>s]`.'

# @!method env_with_bracket?(node)
def_node_matcher :env_with_bracket?, <<~PATTERN
(send (const nil? :ENV) :[] $_)
PATTERN

def on_send(node)
env_with_bracket?(node) do |expression|
break if allowed_var?(expression)
break unless offensive?(node)

add_offense(node, message: format(MSG, key: expression.source)) do |corrector|
corrector.replace(node, "ENV.fetch(#{expression.source}, nil)")
end
end
end

private

def allowed_var?(expression)
expression.str_type? && cop_config['AllowedVars'].include?(expression.value)
end

# rubocop:disable Metrics/CyclomaticComplexity
def offensive?(node)
only_node_of_expression?(node) ||
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think it'd be a good idea to add some explanatory comment here, as it's not very clear why all those checks are needed.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Great point. I will write some comment for that.

BTW, this cop ignore some ENV[] in the below cases;

  • when an ENV[] is no more than a flag for a condition
  • when an ENV[] receives some message (This may be controversial though)

This feature is documented in the also good section of the documentation.
I’m going to mention about above as well, but would like to hear if this idea sounds good for others.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I added some comments and made the logic simpler.
17a52ef

method_argument?(node) ||
array_element?(node) ||
hash_key?(node) ||
compared?(node) ||
case?(node) ||
operand_of_or?(node) ||
last_child_of_parent_node?(node)
end
# rubocop:enable Metrics/CyclomaticComplexity

def case?(node)
node.parent&.case_type? || node.parent&.when_type?
end

def only_node_of_expression?(node)
node.parent.nil?
end

def method_argument?(node)
return false unless node.parent&.send_type?

node.parent.arguments.include?(node)
end

def array_element?(node)
return false unless node.parent&.array_type?

node.parent.children.include?(node)
end

def hash_key?(node)
return false unless node.parent&.pair_type?

node.parent.children.first == node
end

def compared?(node)
return false unless node.parent&.send_type?

node.parent.comparison_method?
end

def operand_of_or?(node)
return false unless node.parent&.or_type?

node.parent.children.include?(node)
end

def last_child_of_parent_node?(node)
return false unless node.parent

node.parent.children.last == node
end
end
end
end
end