Request to change Style/HashSyntax to ruby19 (the default)

[jmkoni]

I have this line in my code:

mount Sidekiq::Web => "/jobs", :as => "jobs", :constraints => Constraints::Staff.new

I wanted to change it to:

mount Sidekiq::Web => "/jobs", as: "jobs", constraints: Constraints::Staff.new

but Standard wouldn't allow it and it's driving me up a wall. I was reading the reason behind the current rule choice and I think ruby19 would fit just as well (encourage 3.1 syntax but allow hash rockets if one isn't a symbol). I don't think this change would make any changes to existing standard projects either, but would allow more flexibility in the future. If not, I'll probably let this go and every time I see this line I'll just cringe. And while @camilopayan told me to say it was his fault, I git blamed and it was definitely @searls.

Also for reference, here is the rule and it's options: https://rubydoc.info/gems/rubocop/RuboCop/Cop/Style/HashSyntax

[jmkoni]

The commit and the reason behind the decision: https://github.com/standardrb/standard/commit/3e2ccfcdd2139ea71c1c005d5ab460790e9232f9

[searls]

Hi Jennifer! I understand your gripe here, and this is 100% my fault, not Cam's.

Since 2018 I have tried to establish as much consistency as possible throughout Standard, and this is one where both solutions are bad, IMO. Cases like yours that use => at all are inconsistent with every other Standard hash globally, so the choice in my mind comes down to whether Standard should keep each hash locally consistent:

• Consistent within a hash: force all pairs in a hash that must have a single => to have all pairs use =>. (The current rule)
• Inconsistent within a hash: force all pairs that require a => to use it, and force all the rest to use : (Your proposal)

I favor keeping this as-is for two reasons:

1. Two "wrongs" don't make a right, so if a hash must buck the global rule then I'd rather keep it internally consistent
2. It is sufficiently rare (and from a type safety perspective, risky) for a hash to mix symbol and object keys that having them visually jump off the page (by using => for every pair) has the added benefit of bringing awareness to the fact the key types are heterogenous. For example, in a large hash, if I add a foo: 5 and it gets converted to :foo => 5, then it'll draw my attention to the hash entry with a non-symbol key, and I'll be immediately aware it's not necessarily safe to iterate over the hash with the assumption all keys are symbols

Open to more discussion

[jmkoni]

Just to be clear, you are thinking that keeping the rule as is will be safer for code bases that might have hashes like this:

{
   a: 2,
   b: 3,
   "c" => 4,
   d: 5
}

and you want to ensure they don't get looping assuming every key is a symbol? Still annoyed, but I'm fine closing in the name of saving others from foolishness.

[jmkoni]

Even if my code does look uglier and there's nothing I can do about it other than make an exception which will just make it uglier 😝

[stevenharman]

git blamed and it was definitely @searls.

I literally laughed, out loud. 😂 APPROVED!

[TALlama]

I just made this change in one of UC's (big, internal) Rails apps. I disabled this cop for the whole routes.rb file for precisely the reason OP mentions: the routing DSL requires some String keys but I wanted to use symbols for everything else. I think this rule as implemented is broadly good, but maybe an exception for that one file could be added?

[tenderlove]

It would be nice if you could differentiate hashes passed to a method call vs just hash literals. IMO the rule makes sense for hash literals, but maybe not so much for hashes passed to a method call 🤷🏻‍♀️*.

The code in routes.rb is supposed to be a DSL which I think is a good reason to exempt it from normal Ruby rules. I doubt Standard could know that you're processing a Rails routes file as opposed to some other file named routes.rb though.

*On the other other hand, you can't do code like this:

def foo(args, a:)
end

foo("a" => "b", a: 123)

So IMO the only time it makes sense to mix hash key types is for DSLs.

[tenderlove]

Honestly mixing hash keys like this is kind of a pain for the routes file implementation as well. If I may, I'd encourage people to use the to: kwarg like so:

mount Sidekiq::Web, to: "/jobs", as: "jobs", constraints: Constraints::Staff.new

[mellowfish]

I only run into this in routes files and after blanket disabling the rule for that file for a while I gave in and just fixed them all. I do hate the :as => foo syntax, but the whole point of standard is to remove thinking about stuff.

[Epigene]

I'm with @searls on this one specifically because of the "consistent within a hash" argument. Sidesteps a sizeable pile of trouble down the line.

[searls]

I'll be totally honest, I didn't even know that routes.rb still supported mount Sidekiq::Web => "/jobs" invocations (I always use to:), and it really feels like a Bad™ thing for a DSL to split up hash and kwargs inputs like that.

If anything, the fact the only counter-example being raised here is routes.rb is making me feel more like the rule as-is identifies a code smell in the method (which should really be fixed in rails but whatcha gonna do)

[benjamineskola]

Personally I think I’d rather not use => more than absolutely necessary. I suppose that’s partly just an aesthetic thing, but if you wanted to make an argument from consistency: perhaps it’s better for all hashes to be as consistent as possible (use the newer syntax) and only deviate from that when absolutely necessary? If you have dozens of new-style hashes across the codebase, and one that has a single key that needs the old-style syntax, isn’t it just making it less consistent to force that whole hash to use the old-style syntax?

or put another way, there’s going to be inconsistency either way, and forcing people to use the less-preferred syntax doesn’t actually get rid of the inconsistency.