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

Best way to apply to all rbi files in the project #94

Closed
RobinDaugherty opened this issue Feb 10, 2022 · 12 comments
Closed

Best way to apply to all rbi files in the project #94

RobinDaugherty opened this issue Feb 10, 2022 · 12 comments

Comments

@RobinDaugherty
Copy link

This is not strictly an issue with rubocop-sorbet, but I've been unable to find a general solution to this problem, and it keeps us from being able to use rubocop-sorbet.

The issue being that once **/*.rbi is added to the Include list for rubocop, it causes all of the other configured cops to run on rbi files, making a lot of complaints (and changes). And of course rbi generation going forward will continue to create them as-is, so in my mind it's not desirable to enforce the same rubocop rules within rbi files.

My preference would be to only include rbi files for the Sorbet cops, and exclude them for all other cops, but there's no elegant way to do this.

Can anyone provide some guidance on this? I can open a PR to add to the README if we can come up with a solution.

@RobinDaugherty RobinDaugherty changed the title Adding rubocop-sorbet causes all other cops to include rbi files Adding rubocop-sorbet to rubocop config causes all other cops to include rbi files Feb 10, 2022
@Morriar
Copy link
Contributor

Morriar commented Feb 11, 2022

👋 Hey Robin,

We provide two config within this suite of cops:

  1. default.yml: the config to use for your application files
  2. rbi.yml: the config to use for your shim files (should only be enabled for the sorbet/rbi/shims/ directory)

Which one is causing you trouble? From the inclusion of **/*.rbi you mention it seems you're trying to enable the rbi.yml config?

@RobinDaugherty
Copy link
Author

RobinDaugherty commented Feb 13, 2022

I tried both...

  • I added rbi.yml to my existing config, and my existing cops applied to all of the rbi files in the project (not desirable).
  • I added default.yml to my existing config, it's able to run the sorbet-related cops on my other files, but then rbi files don't get checked at all.
    • If I then add sorbet/rbi/shims/.rubocop.yml, it's able to see rbi files within sorbet/rbi/shims, but then Sorbet/ForbidRBIOutsideOfAllowedPaths doesn't do any good because it can't see rbi files outside of sorbet/rbi/shims.

If I understand correctly, the only way I can get Sorbet/ForbidRBIOutsideOfAllowedPaths to apply to rbi files in the whole project would be to add the rbi pattern to AllCops.Include. But if do that, then all of my existing cops will be applied to the rbi files, which I don't want.

Is there any path forward besides adding **/*.rbi to AllCops.Include and then also to Exclude for all of the other cops?

@RobinDaugherty RobinDaugherty changed the title Adding rubocop-sorbet to rubocop config causes all other cops to include rbi files Best way to apply to all rbi files in the project Feb 13, 2022
@Morriar
Copy link
Contributor

Morriar commented Feb 14, 2022

I think your top level Rubocop configuration should only use default.yml as it contains the settings expected for Ruby files.

If you want to apply the rbi.yml configuration you can add a .rubocop.yml in your sorbet/rbi/ directory. You can find an example in Tapioca.

@RobinDaugherty
Copy link
Author

That makes sense, though I think the README could be more clear about that.

But doesn't this leave us with rbi-related cops not seeing rbi files that are outside sorbet/rbi, including Sorbet/ForbidRBIOutsideOfAllowedPaths, whose express purpose is to look for rbi files that exist outside of sorbet/rbi?

@Morriar
Copy link
Contributor

Morriar commented Feb 14, 2022

The default.yml config already contains the cops that make sense for your top level. For example Sorbet/ForbidRBIOutsideOfAllowedPaths is setup in default.yml.

But yeah I agree we can make the README clearer. I'll push a change 👍

@RobinDaugherty
Copy link
Author

RobinDaugherty commented Feb 14, 2022

For example Sorbet/ForbidRBIOutsideOfAllowedPaths is setup in default.yml.

So I might just be experiencing an issue caused by my Rubocop config, but it doesn't seem to be working as I expect from your description.

I have a file app/models/mymodel.rbi. If I run rubocop I don't get any complaints from Sorbet/ForbidRBIOutsideOfAllowedPaths, but if I run rubocop app/models/mymodel.rbi I get a complaint from Sorbet/ForbidRBIOutsideOfAllowedPaths (along with complaints from all of the other cops enabled in my top-level rubocop config).

My understanding of this is: rbi files aren't Included in the top-level rubocop config, so this cop won't see them when running with the top-level config. But specifying a file at the command line bypasses AllCops.Include patterns (and possibly each cop's Include pattern still applies).

I have sorbet/rbi/.rubocop.yml like this:

require:
  - rubocop-sorbet

inherit_gem:
  rubocop-sorbet: config/rbi.yml

My top-level project .rubocop.yml:

require:
  - standard
  - rubocop-rails
  - rubocop-sorbet

inherit_gem:
  standard: config/base.yml
  rubocop-rails: config/default.yml
  rubocop-sorbet: config/default.yml

AllCops:
  TargetRubyVersion: 3.0
  Exclude:
    - "Gemfile.lock"
    - "bin/*"
    - "config.ru"

When I run rubocop in the root of the project, I get no complaints from Sorbet/ForbidRBIOutsideOfAllowedPaths. When I cd sorbet/rbi and run rubocop, I get complaints seemingly from all default cops. When I debug, I get the following:

For /myproject/sorbet/rbi: configuration from /myproject/sorbet/rbi/.rubocop.yml
configuration from /opt/rbenv/versions/3.0.3/lib/ruby/gems/3.0.0/gems/rubocop-sorbet-0.6.6/config/default.yml
configuration from /opt/rbenv/versions/3.0.3/lib/ruby/gems/3.0.0/gems/rubocop-sorbet-0.6.6/config/default.yml
Default configuration from /opt/rbenv/versions/3.0.3/lib/ruby/gems/3.0.0/gems/rubocop-1.25.0/config/default.yml
Inheriting configuration from /opt/rbenv/versions/3.0.3/lib/ruby/gems/3.0.0/gems/rubocop-sorbet-0.6.6/config/rbi.yml
AllCops/Exclude configuration from /myproject/.rubocop.yml
configuration from /opt/rbenv/versions/3.0.3/lib/ruby/gems/3.0.0/gems/rubocop-rails-2.13.2/config/default.yml
configuration from /opt/rbenv/versions/3.0.3/lib/ruby/gems/3.0.0/gems/rubocop-rails-2.13.2/config/default.yml
Inheriting configuration from /opt/rbenv/versions/3.0.3/lib/ruby/gems/3.0.0/gems/rubocop-sorbet-0.6.6/config/default.yml
/opt/rbenv/versions/3.0.3/lib/ruby/gems/3.0.0/gems/rubocop-sorbet-0.6.6/config/default.yml: Sorbet/sigils/EnforceSingleSigil has the wrong namespace - should be Sorbet
Inheriting configuration from /opt/rbenv/versions/3.0.3/lib/ruby/gems/3.0.0/gems/rubocop-rails-2.13.2/config/default.yml
Inheriting configuration from /opt/rbenv/versions/3.0.3/lib/ruby/gems/3.0.0/gems/standard-1.7.0/config/base.yml
configuration from /opt/rbenv/versions/3.0.3/lib/ruby/gems/3.0.0/gems/rubocop-performance-1.13.2/config/default.yml
configuration from /opt/rbenv/versions/3.0.3/lib/ruby/gems/3.0.0/gems/rubocop-performance-1.13.2/config/default.yml

So it seems to inherit the entire top-level config in the subdirectory no matter what. I can't get it to run correctly from either the top level (it's ignoring all rbi files) or within the sorbet/rbi subdir (it's applying all of the non-rbi rules to the generated rbi files).

Looking at the Tapioca project, the top-level rubocop config doesn't seem to use rubocop-sorbet's default.yml config, and that top-level config adds an Include pattern "sorbet/rbi/**/*.rbi". I wonder if that's necessary for the sorbet/rbi/.rubocop.yml config to be apply to rbi files at all.

And I wonder if the tapioca project is applying all of the standard rules to the contents of the rbi files (which is what I'm working to void). When I have some more time I'll pull down the tapioca repo and do some experimentation with its rubocop config.

@Morriar
Copy link
Contributor

Morriar commented Feb 15, 2022

I've been testing this with this setup:

.rubocop.yml
Gemfile
lib
  file.rb
  file.rbi
sorbet
  rbi
    .rubocop.yml
    file.rbi

Where .rubocop.yml contains this configuration:

require:
  - rubocop-sorbet

inherit_gem:
  rubocop-sorbet: config/default.yml

AllCops:
  Include:
    - "**/*.rbi"

And sorbet/rbi/.rubocop.yml contains this:

require:
  - rubocop-sorbet

inherit_gem:
  rubocop-sorbet: config/rbi.yml

And finally running rubocop:

$ bundle exec rubocop

Inspecting 2 files
CC

Offenses:

lib/file.rbi:1:1: C: Sorbet/ForbidRBIOutsideOfAllowedPaths: RBI file path should match one of: sorbet/rbi/**
# typed: true
^
sorbet/rbi/file.rbi:3:1: C: [Correctable] Sorbet/SingleLineRbiClassModuleDefinitions: Empty class/module definitions in RBI files should be on a single line.
class Foo ...
^^^^^^^^^

2 files inspected, 2 offenses detected, 1 offense auto-correctable

Would that work with your project?

@RobinDaugherty
Copy link
Author

RobinDaugherty commented Mar 3, 2022

Adding the following to .rubocop.yml:

AllCops:
  Include:
    - "**/*.rbi"

causes all configured cops to evaluate rbi files. In my case, and I suspect in most projects, it causes those other cops to find a lot of style, layout, and other complaints in generated rbi files.

I see two solutions:

  1. Add an Exclude of **/*.rbi to every other enabled cop in the top-level config
  2. Run rubocop twice. First time with the default configuration file, second time specifying a different config file that includes rbi files and inherits from rubocop-sorbet's config/default.yml.

@Morriar
Copy link
Contributor

Morriar commented Mar 10, 2022

👋 Hey Robin,

I might be missing something from your configuration because I'm unable to reproduce what you're saying.

Let's take this demo repo as example:

  • We created a .rubocop.yml root configuration including both .rb and .rbi files
  • As well as a sorbet/rbi/.rubocop.yml configuration file to import the rbi specific rules

As you can see when you run bundle exec rubocop you do get the expected errors:

$ bundle exec rubocop
Inspecting 3 files
CCC

Offenses:

lib/file.rb:1:1: C: [Correctable] Style/FrozenStringLiteralComment: Missing frozen string literal comment.
# typed: true
^

lib/file.rb:5:1: C: [Correctable] Layout/TrailingEmptyLines: 1 trailing blank lines detected.

lib/file.rbi:1:1: C: Sorbet/ForbidRBIOutsideOfAllowedPaths: RBI file path should match one of: sorbet/rbi/**
# typed: true
^

lib/file.rbi:4:7: C: [Correctable] Style/ClassAndModuleChildren: Use nested module/class definitions instead of compact style.
class Foo::Bar
      ^^^^^^^^

sorbet/rbi/file.rbi:3:1: C: [Correctable] Sorbet/SingleLineRbiClassModuleDefinitions: Empty class/module definitions in RBI files should be on a single line.
class Foo::Bar ...
^^^^^^^^^^^^^^

3 files inspected, 5 offenses detected, 4 offenses auto-correctable

As you can see, no rbi.yml related cops are reporting offenses from my normal Ruby files and AllCops offenses such as Style/FrozenStringLiteralComment or Style/ClassAndModuleChildren are not reported from inside the sorbet/rbi directory.

Note that a Style/ClassAndModuleChildren offense is still reported for lib/file.rbi because it is in the included path for AllCops, but this offense will disappear as soon as we move the .rbi file to the proper location as reported by Sorbet/ForbidRBIOutsideOfAllowedPaths.

Do you mind trying with this demo repo and validate that we're seeing the same behavior?

I hope it helps 🙏

@RobinDaugherty
Copy link
Author

Thank you @Morriar! I was able to verify that the demo repo works as expected. I want to deeper analyze the differences with my project, but I'll go ahead and close this for now. I really appreciate your time on this!

@RobinDaugherty
Copy link
Author

RobinDaugherty commented May 1, 2022

I was able to give this another look with the benefit of more experience with Sorbet. The base issue that confused me is that though I'm using srb rbi to generate my RBI files, the rubocop configuration provided by this project for RBI files complains about those newly-generated files. In other words this project enforces a style for RBI files that is very different from the RBI code generated by Sorbet.

I understand now that Shopify is using Tapioca to generate/maintain RBI files, so that is not wholly unexpected. I hope to get Tapioca working in my Rails project in the future. Rather than apply tens of thousands of autofixes and continue to maintain those (which I couldn't actually do because it crashed rubocop), I chose to add sorbet/rbi to the Exclude list.

@Morriar
Copy link
Contributor

Morriar commented May 2, 2022

👋 Hey Robin,

I should have added that most of this rules are too enforce styling on manually written RBI files (called shims). We generally exclude auto-generated files from Rubocop.

Thanks for the feedback!

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

No branches or pull requests

2 participants