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 #9621] Add consistent style to Lint/SymbolConversion to require all symbol keys in a hash to use the same convention #9623

Merged
merged 1 commit into from Mar 22, 2021
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Jump to
Jump to file
Failed to load files.
Diff view
Diff view
4 changes: 4 additions & 0 deletions config/default.yml
Expand Up @@ -2063,6 +2063,10 @@ Lint/SymbolConversion:
Description: 'Checks for unnecessary symbol conversions.'
Enabled: pending
VersionAdded: '1.9'
EnforcedStyle: strict
SupportedStyles:
- strict
- consistent

Lint/Syntax:
Description: 'Checks for syntax errors.'
Expand Down
91 changes: 89 additions & 2 deletions lib/rubocop/cop/lint/symbol_conversion.rb
Expand Up @@ -6,6 +6,12 @@ module Lint
# This cop checks for uses of literal strings converted to
# a symbol where a literal symbol could be used instead.
#
# There are two possible styles for this cop.
# `strict` (default) will register an offense for any incorrect usage.
# `consistent` additionally requires hashes to use the same style for
# every symbol key (ie. if any symbol key needs to be quoted it requires
# all keys to be quoted).
#
# @example
# # bad
# 'string'.to_sym
Expand All @@ -21,10 +27,49 @@ module Lint
# :underscored_symbol
# :'hyphenated-string'
#
# @example EnforcedStyle: strict (default)
#
# # bad
# {
# 'a': 1,
# "b": 2,
# 'c-d': 3
# }
#
# # good (don't quote keys that don't require quoting)
# {
# a: 1,
# b: 2,
# 'c-d': 3
# }
#
# @example EnforcedStyle: consistent
Copy link
Collaborator

Choose a reason for hiding this comment

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

Might be a good idea to add the strict case for hashes as well, so it's clearer what this means for them.

Copy link
Member Author

Choose a reason for hiding this comment

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

@bbatsov I added additional documentation, thanks!

#
# # bad
# {
# a: 1,
# 'b-c': 2
# }
#
# # good (quote all keys if any need quoting)
# {
# 'a': 1,
# 'b-c': 2
# }
#
# # good (no quoting required)
# {
# a: 1,
# b: 2
# }
#
class SymbolConversion < Base
extend AutoCorrector
include ConfigurableEnforcedStyle

MSG = 'Unnecessary symbol conversion; use `%<correction>s` instead.'
MSG_CONSISTENCY = 'Symbol hash key should be quoted for consistency; ' \
'use `%<correction>s` instead.'
RESTRICT_ON_SEND = %i[to_sym intern].freeze

def on_send(node)
Expand All @@ -35,7 +80,7 @@ def on_send(node)
end

def on_sym(node)
return if properly_quoted?(node.source, node.value.inspect)
return if ignored_node?(node) || properly_quoted?(node.source, node.value.inspect)

# `alias` arguments are symbols but since a symbol that requires
# being quoted is not a valid method identifier, it can be ignored
Expand All @@ -51,6 +96,21 @@ def on_sym(node)
register_offense(node, correction: node.value.inspect)
end

def on_hash(node)
# For `EnforcedStyle: strict`, hash keys are evaluated in `on_sym`
return unless style == :consistent

keys = node.keys.select(&:sym_type?)

if keys.any? { |key| requires_quotes?(key) }
correct_inconsistent_hash_keys(keys)
else
# If there are no symbol keys requiring quoting,
# treat the hash like `EnforcedStyle: strict`.
keys.each { |key| correct_hash_key(key) }
end
end

private

def register_offense(node, correction:, message: format(MSG, correction: correction))
Expand All @@ -60,14 +120,18 @@ def register_offense(node, correction:, message: format(MSG, correction: correct
end

def properly_quoted?(source, value)
return true if !source.match?(/['"]/) || value.end_with?('=')
return true if style == :strict && (!source.match?(/['"]/) || value.end_with?('='))

source == value ||
# `Symbol#inspect` uses double quotes, but allow single-quoted
# symbols to work as well.
source.tr("'", '"') == value
end

def requires_quotes?(sym_node)
sym_node.value.inspect.match?(/^:".*?"|=$/)
end

def in_alias?(node)
node.parent&.alias_type?
end
Expand Down Expand Up @@ -97,6 +161,29 @@ def correct_hash_key(node)
message: format(MSG, correction: "#{correction}:")
)
end

def correct_inconsistent_hash_keys(keys)
keys.each do |key|
ignore_node(key)

next if requires_quotes?(key)
next if properly_quoted?(key.source, %("#{key.value}"))

correction = "#{quote_type}#{key.value}#{quote_type}"
register_offense(
key,
correction: correction,
message: format(MSG_CONSISTENCY, correction: "#{correction}:")
)
end
end

def quote_type
# Use the `Style/StringLiterals` configuration for quoting symbols
return '"' unless config.for_cop('Style/StringLiterals')['Enabled']

config.for_cop('Style/StringLiterals')['EnforcedStyle'] == 'single_quotes' ? "'" : '"'
end
end
end
end
Expand Down
150 changes: 150 additions & 0 deletions spec/rubocop/cop/lint/symbol_conversion_spec.rb
Expand Up @@ -168,4 +168,154 @@
RUBY
end
end

context 'EnforcedStyle: consistent' do
let(:string_literals_enabled) { true }
let(:string_literals_style) { 'single_quotes' }

let(:cop_config) { { 'EnforcedStyle' => 'consistent' } }
let(:other_cops) do
{
'Style/StringLiterals' => {
'Enabled' => string_literals_enabled,
'EnforcedStyle' => string_literals_style
}
}
end

context 'hash where no keys need to be quoted' do
it 'does not register an offense' do
expect_no_offenses(<<~RUBY)
{
a: 1,
b: 2
}
RUBY
end
end

context 'hash where keys are quoted but do not need to be' do
it 'registers an offense' do
expect_offense(<<~RUBY)
{
a: 1,
'b': 2
^^^ Unnecessary symbol conversion; use `b:` instead.
}
RUBY

expect_correction(<<~RUBY)
{
a: 1,
b: 2
}
RUBY
end
end

context 'hash where there are keys needing quoting' do
it 'registers an offense for unquoted keys' do
expect_offense(<<~RUBY)
{
a: 1,
^ Symbol hash key should be quoted for consistency; use `'a':` instead.
b: 2,
^ Symbol hash key should be quoted for consistency; use `'b':` instead.
'c': 3,
'd-e': 4
}
RUBY

expect_correction(<<~RUBY)
{
'a': 1,
'b': 2,
'c': 3,
'd-e': 4
}
RUBY
end
end

context 'with a key with =' do
it 'requires symbols to be quoted' do
expect_offense(<<~RUBY)
{
'a=': 1,
b: 2
^ Symbol hash key should be quoted for consistency; use `'b':` instead.
}
RUBY

expect_correction(<<~RUBY)
{
'a=': 1,
'b': 2
}
RUBY
end
end

context 'with different quote styles' do
it 'does not register an offense' do
expect_no_offenses(<<~RUBY)
{
'a': 1,
"b": 2,
"c-d": 3,
'e-f': 4
}
RUBY
end
end

context 'with a mix of string and symbol keys' do
it 'does not register an offense' do
expect_no_offenses(<<~RUBY)
{
'a' => 1,
'b-c': 2
}
RUBY
end
end

context 'quote style' do
let(:source) do
<<~RUBY
{ a: 1, 'b-c': 2 }
RUBY
end

context 'when Style/StringLiterals is not enabled' do
let(:string_literals_enabled) { false }

it 'uses double quotes to correct' do
expect_correction(<<~RUBY, source: source)
{ "a": 1, 'b-c': 2 }
RUBY
end
end

context 'when Style/StringLiterals uses single_quotes style' do
let(:string_literals_style) { 'single_quotes' }

it 'uses double quotes to correct' do
expect_correction(<<~RUBY, source: source)
{ 'a': 1, 'b-c': 2 }
RUBY
end
end

context 'when Style/StringLiterals uses double_quotes style' do
let(:string_literals_style) { 'double_quotes' }

it 'uses double quotes to correct' do
expect_correction(<<~RUBY, source: source)
{ "a": 1, 'b-c': 2 }
RUBY
end
end
end
end
end