Hi friends!

Just updated rubocop, and I now get a warning because Style/ArrayIntersect is not configured. I could configure it on my side but would rather just have a release of ruby-style-guide that does it for me (as done by @sambostock in #487).

If you could cut a new release, that would be most appreciated!

Ruby Version: 2.6.9

GemEnv

RubyGems Environment:
  - RUBYGEMS VERSION: 3.0.3.1
  - RUBY VERSION: 2.6.9 (2021-11-24 patchlevel 207) [x86_64-linux]
  - INSTALLATION DIRECTORY: /home/lance/.gem/ruby/2.6.9
  - USER INSTALLATION DIRECTORY: /home/lance/.gem/ruby/2.6.0
  - RUBY EXECUTABLE: /home/lance/.rubies/ruby-2.6.9/bin/ruby
  - GIT EXECUTABLE: /usr/bin/git
  - EXECUTABLE DIRECTORY: /home/lance/.gem/ruby/2.6.9/bin
  - SPEC CACHE DIRECTORY: /home/lance/.gem/specs
  - SYSTEM CONFIGURATION DIRECTORY: /home/lance/.rubies/ruby-2.6.9/etc
  - RUBYGEMS PLATFORMS:
    - ruby
    - x86_64-linux
  - GEM PATHS:
     - /home/lance/.gem/ruby/2.6.9
     - /home/lance/.rubies/ruby-2.6.9/lib/ruby/gems/2.6.0
  - GEM CONFIGURATION:
     - :update_sources => true
     - :verbose => true
     - :backtrace => false
     - :bulk_threshold => 1000
     - :sources => ["https://rubygems.org/"]
     - :concurrent_downloads => 8
     - "gem" => "--document=yri"
  - REMOTE SOURCES:
     - https://rubygems.org/
  - SHELL PATH:
     - /home/lance/.gem/ruby/2.6.9/bin
     - /home/lance/.rubies/ruby-2.6.9/lib/ruby/gems/2.6.0/bin
     - /home/lance/.rubies/ruby-2.6.9/bin
     - /home/lance/.local/bin
     - /home/lance/.nvm/versions/node/v14.19.1/bin
     - /home/lance/.local/bin
     - /usr/local/sbin
     - /usr/local/bin
     - /usr/sbin
     - /usr/bin
     - /sbin
     - /bin
     - /usr/games
     - /usr/local/games
     - /snap/bin
     - /home/lance/dev/bin/

Added just this gem (no other rubocop gems) to my project and get this error.

bundle exec rubocop
Error: The `Gemspec/DateAssignment` cop has been removed. Please use `Gemspec/DeprecatedAttributeAssignment` instead.
(obsolete configuration found in /home/lance/.gem/ruby/2.6.9/gems/rubocop-shopify-2.5.0/rubocop.yml, please update it)


gem list rubocop      

*** LOCAL GEMS ***

rubocop (1.31.0)
rubocop-ast (1.24.0)
rubocop-rspec (2.11.1)
rubocop-shopify (2.5.0)

Encountered a few issues trying to fix the style for spy. Those seem relatively minor but each change needs a lot more reflexion than I expected.

Time.parse doesn't end up with the same time zone

The 00:00 timezone information is not parsed the same way. Time seems to ignore the time zone information from the argument in this case (local time zone is currently UTC-4):

[2] pry(main)> DateTime.parse('2017-08-08T15:00:00 00:00').utc.iso8601
=> "2017-08-08T15:00:00+00:00"
[3] pry(main)> Time.parse('2017-08-08T15:00:00 00:00').utc.iso8601
=> "2017-08-08T19:00:00Z"

In this case Time.zone.parse is a more suitable replacement for DateTime.rfc3339.

Time.iso8601 prints UTC time zone differently

[1] pry(main)> DateTime.parse('2017-08-08T15:00:00Z').iso8601
=> "2017-08-08T15:00:00+00:00"
[2] pry(main)> Time.parse('2017-08-08T15:00:00Z').iso8601
=> "2017-08-08T15:00:00Z"

This is mostly harmless, but sometimes makes tests fail.

Date.rfc3339 timezone information is lost

[4] pry(main)> DateTime.rfc3339('2002-10-02T10:00:00-05:00').to_time.utc
=> 2002-10-02 15:00:00 UTC
[5] pry(main)> Date.rfc3339('2002-10-02T10:00:00-05:00').to_time.utc
=> 2002-10-02 04:00:00 UTC

Again, Time.zone.parse is a more suitable replacement for DateTime.rfc3339.

Just noticed there's a warning showing up while running rubocop due to a deprecated(?) IgnoredPatterns param.

❯ rubocop
Warning: obsolete parameter `IgnoredPatterns` (for `Layout/LineLength`) found in /Users/larouxn/.gem/ruby/3.1.2/gems/rubocop-shopify-2.5.0/rubocop.yml
`IgnoredPatterns` has been renamed to `AllowedPatterns`.
Warning: obsolete parameter `IgnoredPatterns` (for `Layout/LineLength`) found in .rubocop.yml
`IgnoredPatterns` has been renamed to `AllowedPatterns`.
Inspecting 2166 files

Not a huge deal, but I thought I'd throw this out there.

I really like the way AirBnB does their Javascript Style Guide, for the following reasons:

• Table of Contents 📄
• Linking to specific sections 👈
• Examples for almost everything, often with 😇 good vs 👿 bad
• "Why"s on many style rules 🤔

In my opinion, these make the style guide much more approachable to developers new to a language, as well as making the rules easier to share with others/refer to. I like having a "why" too, because rules make so much more sense when they aren't arbitrary.

Right now we have this rule disabled: http://www.rubydoc.info/gems/rubocop/RuboCop/Cop/Style/TrailingCommaInLiteral

This cop checks for trailing comma in array and hash literals.

This is something that frequently gets called out in PRs as trailing commas are generally accepted as a good practice for multiline literals. Right now we have nothing enforcing it one way or another.

Is there a compelling reason why we shouldn't use this rule to enforce the usage?

cc @nickhoffman

Hello friends,
I'm opening this issue to request disabling the Style/ClassAndModuleChildren for files in a component's test folder.

do you think such cop makes sense?

(following up from internal Slack discussion)

This rule is disabled by default in RuboCop's config. It was enabled in #486.

It's causing some problems since it can contradict Style/NegatedIf.

Also it does not match the styleguide rule for "Favour unless over if for negative conditions".

In some of our repos we are starting to get prompted for adding the Style/ArrayIntersect cop. Seems like this new cop was added fairly recently (v1.40):

The following cops were added to RuboCop, but are not configured. Please set Enabled to either `true` or `false` in your `.rubocop.yml` file.

Please also note that you can opt-in to new cops by default by adding this to your config:
  AllCops:
    NewCops: enable

Style/ArrayIntersect: # new in 1.40
  Enabled: true

What is the recommendation for adding new cops not covered by our centralized rubocop.yml styleguide? This came up after a recent dependabot updates for rubocop-shopify

Extracted from #74. This would allow us to link to rules directly.

cc @sambostock

https://github.com/Shopify/ruby-style-guide/blob/gh-pages/README.md says use find over detect but I find this a little awkward on AR associations especially since it has potential DB implications if not provided with a block.

My tendency is still to use detect in this case, but then you get into inconsistent scenarios when it's not an AR association.

Thoughts? /cc @christianblais

Policial is complaining that there is implicit string concatenation (two strings beside each other will be concatenated in Ruby):

However, this is actually just a triple-quoted string, and NOT implicit string concatenation.

Rubocop will always enforce ruby's 3.1 hash key shorthand syntax by default. Do we want to change the default behavior?

The Hash Literal Values says this is good.
For what its worth, Standard is set to either.

Weak opinion: My preference would be to set it to either. Allow its use but not require it.

Is there a plan for making Policial no longer advisory and actually something that needs to pass before merging? If not I think there should be.

• It's important to be able to know if your PR is safe to merge at a glance. We want to be able to ship fast, and having to click in to see what the issues are is slow. For the same reasons that false CI failures are so annoying, false lint failures should be just as important.
• It's important to have a consistently applied set of style rules. Anecdotally, it seems like everyone has their own set of rules they consider actual rules, and a bunch more that they consider guidelines and will still merge when red. These two sets of rules are a little different for each person. So, we're tending towards writing similar code, but never quite making it.
• It's important for onboarding to not have broken windows and tribal knowledge. Which rules are rules and which are guidelines is something people just seem to make up, and this is intimidating / unknowable for people new to the company. 30% of developers joined in the past 5 months.

@volmer @kirs ... who else should I tag?

Extracted from #74. It’s not clear however which rules require the « why » to be explicit in the style guide. I fear that explaining the whole context behind a rule would make the document way too long, and developers can access the PR discussions to learn more about it via git blame.

cc @sambostock

Extracted from #74. It would be great to know exactly which rules would be made easier to understand with more examples. Some are simple and straightforward enough and don’t need examples.

cc @sambostock

Has the decision to enforce extend_self over module_function ever been revisited internally?

The whole point of module_function is to allow namespaced stateless methods to be available as public class methods, while simultaneously included as private helper methods (when those methods are oft-used in a particular class) without polluting the class' public api. extend self isn't compatible with module_function because the visibility of the methods is forced to be the same at both the class and instance level.

When extend self; private is used to avoid polluting the class' instance api, then you can't invoke them statically. But if you make them public so they can be namespaced utility methods, then they pollute the class' public api!

This implies these two approaches aren't solving the same problem. module_function is purpose built to have different visibility. extend self is an idiom that is used for a variety of other purposes that only appears to overlap with module_function. But it has a bigger ramification than was likely recognized when it become oft-used/cargo-culted.

I dug into the history of the cop's setting. It seems the initial decision was at least partially (if not primarily?) about class << self vs def self.. Nothing in the issue thread speaks to the nuance that module_function provides; wherein methods are desired to have different visibilities because they serve different purposes (public as class; private as instance). Since the initial discussion didn't seem to take these different purposes and behaviors into account, I'm hoping to at least broach that topic here.

Extracted from #74, having a Table of Contents in the beginning of the document would make it easier to read.

cc @sambostock

Parts of this guide are derived from the Ruby Style Guide which has a Creative Commons Attribution License.

I have updated rubocop-shopify to 2.11.1 and fixing the layout with bundle exec rubocop -A produced some weird changes.

The code before:

    def collection
      Service.not_deleted.includes(
        { vault_teams:
            [:github_team_vault_teams, :teams] },
        { app:
            [{ runtimes:
                 [:runtime_instances, :app] },
             :runtimes,
             :repo,] },
        :repo,
        :app,
        :monorepo_mobile_app,
        :bugsnag_project,
        :slack_channels,
        :vault_teams,
        :vault_users,
      ).joins(:app)
    end

The code after:

    def collection
      Service.not_deleted.includes(
        {
          vault_teams:
                      [:github_team_vault_teams, :teams],
        },
        {
          app:
                      [
                        {
                          runtimes:
                                           [:runtime_instances, :app],
                        },
                        :runtimes,
                        :repo,
                      ],
        },
        :repo,
        :app,
        :monorepo_mobile_app,
        :bugsnag_project,
        :slack_channels,
        :vault_teams,
        :vault_users,
      ).joins(:app)
    end

There are more extreme examples in this file.

It seems the problem doesn't happen if the opening bracket (or whole entry) is on the same line:

Before

    def collection
      Service.not_deleted.includes(
        { vault_teams:[:github_team_vault_teams, :teams] },
        { app:[
          { runtimes: [:runtime_instances, :app] },
             :runtimes,
             :repo,] },
        :repo,
        :app,
        :monorepo_mobile_app,
        :bugsnag_project,
        :slack_channels,
        :vault_teams,
        :vault_users,
      ).joins(:app)
    end

After

    def collection
      Service.not_deleted.includes(
        { vault_teams: [:github_team_vault_teams, :teams] },
        {
          app: [
            { runtimes: [:runtime_instances, :app] },
            :runtimes,
            :repo,
          ],
        },
        :repo,
        :app,
        :monorepo_mobile_app,
        :bugsnag_project,
        :slack_channels,
        :vault_teams,
        :vault_users,
      ).joins(:app)
    end

We currently have Style/CaseEquality cop enabled, which disallows the usage of ===.

In general, I agree that === should be discouraged (on Enumerable, Regexp, etc), but I don't think we can disallow it using Rubocop, and in at least one scenario it should actually be the encouraged way of doing things: hierarchy membership checks (aka: typechecking, but not quite).

The idiomatic way of doing hierarchy membership checking is typically using the is_a? or kind_of?, but this should actually be discouraged, for two reasons.

1. BasicObject doesn't respond to is_a? and so whenever we use is_a?.
   When we use is_a? we introduce coupling (which is generally unnecessary), between the code that checks and the code that produces the objects. This practice is also prevented by Sorbet when using interfaces, because the implementation could very well be a BasicObject. In the Checkouts component, we've had to disable the check entirely since we use Sorbet with strict typing enabled.

2. When checking membership, we should always ask the collection rather than the item. E.g., we do array.include?(item) and never item.contained_in?(array).

Continuing on a tangent...

I think we should disable all Rubocop rules that can have different semantics. Rubocop is simply not equipped to know what the right solution should be. It's fine to Rubocop to indent code, add commas, or parentheses, or make sure that a class name fits the file name. However, it shouldn't be used to know which method to use. Another great backwards example is the DynamicFinder cop. Sometimes, your API will have def find_by_id, and it's not an ActiveRecord, so it shouldn't be replaced by find_by(id:). If we used a linter that was aware of the types, we could pull this off, but Rubocop isn't aware of it, and IMO, shouldn't be used here.

TL;DR

It might make sense for us to expose the styleguide as a gem, allowing configuration updates to become part of the predicatable Dependabot auto-update process.

Details

As brought up in Slack

Currently, the way we share our Rubocop config across repos is

# .rubocop.yml
inherit_from:
  - http://shopify.github.io/ruby-style-guide/rubocop.yml

The thing is, this doesn't follow any kind of versioning. It just follows the rules from inheriting from a remote URL

The remote config file is cached locally and is only updated if:

• The file does not exist.
• The file has not been updated in the last 24 hours.
• The remote copy has a newer modification time than the local copy.

Sometimes the file is gitignored, sometimes it isn't.

• If it is ignored, then sometimes new rules start being enforced in CI.
• If it isn't, then sometimes a random PR will have updates to the style config.

Given that we have Dependabot to keep dependencies up to date, and it is possible to include Rubocop config in a gem, would it make sense to publish our config as a gem, and use inherit_gem to apply it?

Other benefits that come to mind are:

• We could explicitly make Rubocop a dependency.
  No more incompatible cops due to version mismatches!
• We could use existing dependency tooling to check for up-to-date style configs (e.g. Services Internal classification checks)
• We could add dependencies on the extracted cop gems (e.g. rubocop-performance), without requiring the client to know to install those cops

If we wanted to get really bold, maybe a gem would even be a good place to put the common foundation for a command similar to dev style --include-branch-commits, instead of every repo implementing it on their own.

cc. @rafaelfranca