Use consistent style for `Lint/SymbolConversion`

[aliismayilov]

I'd like to suggest to use consistent style for Lint/SymbolConversion. Examples from rubocop docs:

# 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
}

[searls]

This is a tricky one, because there are two competing interests here. On one hand, I agree that consistency in hash format is important. For example, if a single => is required in a hash literal, Standard converts all entries to use => so that it and : aren't interspersed.

On the other hand, enforcing that the presence of a single illegal identifier should force all such identifiers to be quoted seems overkill. For example, if you carried the logic that the presence of one illegal symbol literal (:b-c) should require all legal symbol literals to be quoted, you could make the same argument for illegal method names (or any other construct).

I'd definitely disagree with a rule that did this, for example:

# bad
class Foo
  def legal_name
  end

  define_method :'illegal-name' do
  end
end

# good
class Foo
  define_method :legal_name do
  end

  define_method :'illegal-name' do
  end
end

One additional argument against, is that I'm always cautious of rules where a later line could force changes in prior lines. So if the 5th line of a hash literal contains an illegal symbol literal name, it shouldn't cause git churn for lines 1-4 without good reason. I think the =>/: rises to that bar, but I don't think this does.

As a result, I'd be :-1: to making this change.

[luizkowalski]

hey 👋🏻

I'm not sure I get the example. I might be wrong, but it looks like this rule, when using consistent configuration only applies to hashes and alias methods, for example, are skipped so I'm not sure I follow your example. SymbolConversion does not trigger anything for these examples you showed.

[searls]

@luizkowalski no it doesn't. I was using it to illustrate the broader point of why I don't think consistency is an unqualified principle in cases where you're working around illegal identifiers.

[aliismayilov]

This is a tricky one, because there are two competing interests here. On one hand, I agree that consistency in hash format is important. For example, if a single => is required in a hash literal, Standard converts all entries to use => so that it and : aren't interspersed.

On the other hand, enforcing that the presence of a single illegal identifier should force all such identifiers to be quoted seems overkill. For example, if you carried the logic that the presence of one illegal symbol literal (:b-c) should require all legal symbol literals to be quoted, you could make the same argument for illegal method names (or any other construct).

I'd definitely disagree with a rule that did this, for example:

# bad
class Foo
  def legal_name
  end

  define_method :'illegal-name' do
  end
end

# good
class Foo
  define_method :legal_name do
  end

  define_method :'illegal-name' do
  end
end

One additional argument against, is that I'm always cautious of rules where a later line could force changes in prior lines. So if the 5th line of a hash literal contains an illegal symbol literal name, it shouldn't cause git churn for lines 1-4 without good reason. I think the =>/: rises to that bar, but I don't think this does.

As a result, I'd be 👎 to making this change.

Fair points. To be honest, I don't have a good argument other than preferring consistency when reading a hash definition.

Due to configured Style/HashSyntax I can't even manually change

{
  a: 1,
  "b-c": 2
}

to

{
  :"a" => 1,
  :"b-c" => 2
}

🤷

[aliismayilov]

I guess an example from a real project would have more weight to it :) See how it even messes up with syntax highlighting.