Match rubocop-shopify with style guide recommendations

[Schwad]

Background

There is a discrepancy between shopify-rubocop and the Shopify Ruby Styleguide.

The Ruby Styleguide says:

Use a class << self block over def self. when defining class methods, and group them together within a single block.

( pr for background )

There is an option to enable this in rubocop under Style/ClassMethodsDefinitions for self_class. Our current default does the opposite, with the EnforcedStyle being def_self.

This corrects the default.

For Reviewers

This cop is currently set to Enabled: false. If we care enough about this to have an opinion in the styleguide should it be changed to Enabled: true? The def self vs class << self is a long-running question of preference so it might be nice to give clarity on this.

Signed-off-by: Nick Schwaderer [email protected]

[sambostock]

This seems fine, given it doesn't actually enable the cop (which would be a bigger discussion, due to the impact), and just configures the cop so that if enabled it would enforce the style consistent with our recommendation.

Nit about the gem name in the description though 😅

-shopify-rubocop
+rubocop-shopify

[Schwad]

Nit about the gem name in the description though 😅

First-time contributor 🙈 I'll get the name right some day! thanks

[volmer]

Could you please change the config to Enabled: true?

The rule has been added to the Style Guide for years now, so everyone should be writing code that follows it by now, even if RuboCop wasn't able to catch those offences so far. Besides it wouldn't make sense to tweak a cop's config if it's not enabled by default.

[joeldrapper]

Is there a way to enforce this style only when there's more than one class method?

[csalvato]

Is there a way to enforce this style only when there's more than one class method?

For the sake of consistency, I don't think that makes sense, personally.

I actually prefer def self. in general, but if we are going to have an opinion on the style, it should be consistent even with just one class method, imo.

[Schwad]

I actually prefer def self. in general

Me too. My concern is the problem that, for shopify, we have thousands of rubyists and if we do something unusual/different in our style guide and don't back it up with a cop, it's easily missed.

I agree with @volmer - let's enforce it.

The rule has been added to the Style Guide for years now, so everyone should be writing code that follows it by now, even if RuboCop wasn't able to catch those offences so far.

I don't know what I'm allowed to say since this is a public repository. But after a quick search at this org it appears both approaches are equally used. Many thousands of times.

Background

I'm only here because it was flagged up on my PR that I was going against the Shopify Ruby Style Guide 😅 - I was surprised my linter didn't catch the Shopify rule. So here we are.

[Schwad]

test/test_helper.rb:7:3: C: Style/ClassMethodsDefinitions: Use class << self to define a class method.
  def self.warn(message) ...
  ^^^^^^^^^^^^^^^^^^^^^^

Even the style guide is in violation... I'll update that now.

[adrianna-chang-shopify]

❤️

[andyw8]

Should we set this to ignore migrations?

https://buildkite.com/shopify/shopify-graphiql-app/builds/2155#0183c3f3-710f-424a-9fbb-7c6f30ca2979

[andyw8]

(the migration generator from older version of Rails used self.up and self.down - see
https://guides.rubyonrails.org/v2.3/migrations.html)

[rafaelfranca]

That isn't true since Rails 3.1. I don't think we should care about very old Rails versions.