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

Enable Lint/SymbolConversion #265

Open
wants to merge 1 commit into
base: main
Choose a base branch
from

Conversation

sambostock
Copy link
Contributor

What

This enables the Lint/SymbolConversion cop.

Why

A common mistake in Ruby is to assume that wrapping Symbol Hash keys in quotes makes the Hash use String keys.

Erroneous UsageIntended Usage
{
  "these":   1,
  "look":    2,
  "like":    3,
  "strings": 4,
  "but":     5,
  "are":     6,
  "symbols": 7,
}
{
  "these"    => 1,
  "actually" => 2,
  "are"      => 3,
  "strings"  => 4,
  "and"      => 5,
  "not"      => 6,
  "symbols"  => 7,
}

This cop enforces the use of the simplest version of a Symbol possible, as shown in these examples from the documentation

# bad
'string'.to_sym
:symbol.to_sym
'underscored_string'.to_sym
:'underscored_symbol'
'hyphenated-string'.to_sym

# good
:string
:symbol
:underscored_string
:underscored_symbol
:'hyphenated-string'

This will incidentally catch Symbol Hash keys that are needlessly quoted, helping to prevent this mistake. The correction of other Symbols is a bonus.

@volmer
Copy link
Contributor

volmer commented May 13, 2021

Hey! Thanks for the suggestion. Maybe it's just me, but I haven't witnessed the mistake you mentioned. The quoted string notation for hash keys has been around for a while, since Ruby 2.2, and I can't recall seeing its usage when the intention was to have symbols as keys. I'm not saying that the mistake doesn't happen, I'm just trying to be mindful about introducing new rules to the style guide.

@sambostock
Copy link
Contributor Author

I can't recall seeing its usage when the intention was to have symbols as keys

What I've seen is the opposite: someone wants a Hash with String keys, but doesn't realize they need to use the "hash rocket" syntax for that, and ends up with buggy code like

BEVERAGES_ENUM = {
  "Apple Juice":    0,
  "Chocolate Milk": 1,
  "Beer":           2,
}

def beverage?(name)
  BEVERAGES_ENUM.key?(name)
end

beverage?("Apple Juice") # false, because name is String, but key is Symbol

In this example, "Beer": would be an offence with this rule enabled, and resolving that offence would make the bug obvious:

  • # Option 1
    BEVERAGES_ENUM = {
      "Apple Juice":    0, # To a Ruby newcomer, this looks like it could be a String
      "Chocolate Milk": 1,
      Beer:             2, # This is obviously not a String
    }
  • # Option 2
    BEVERAGES_ENUM = {
      "Apple Juice":    0,
      "Chocolate Milk": 1,
      "Beer" =>         2, # This syntax doesn't match the others
    }

In both cases, something now stands out, which might prompt the Ruby newcomer to research Hash keys and eventually change their code to be correct:

BEVERAGES_ENUM = {
  "Apple Juice"    => 0,
  "Chocolate Milk" => 1,
  "Beer"           => 2,
}

Just seems like a simple foot-gun we can get rid of, the same way we've adopted parentheses.


Arguably, this would be caught by writing good tests, but I don't see that as a reason not to enable the rule. Moreover, I don't see any reason why we would want to allow Symbol usage like "foo".to_sym.

@rafaelfranca
Copy link
Member

In this example, "Beer": would be an offence with this rule enabled, and resolving that offence would make the bug obvious:

Only Beer? If it is only it we might not have any advantage of enabling this cop since if "Beer" was a "IPA Beer" instead the entire hash would be valid in the view of this cop.

@sambostock
Copy link
Contributor Author

Yeah, unfortunately I have been unable to find a way to strictly enforce the use of => in hashes with quoted symbol keys, which is why I opened rubocop/rubocop#9728. A comment suggested the use of Lint/SymbolConversion, and I figured it would be better than nothing in the interim, especially since the other things this cop catches seem useful (e.g. calling .to_sym on a String literal instead of using a Symbol literal).

@rafaelfranca
Copy link
Member

Can you check how many violations to this rule we have in Shopify core?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants