Refactor ForbidRBIOutsideOfAllowedPaths & rename to RBIFilePath

[sambostock]

This makes the following changes:

• Refactors ForbidRBIOutsideOfAllowedPaths specs
  This refactors the specs to be more idiomatic, passing the filename to the expect_offense and expect_no_offenses helpers, instead of stubbing the filename. It also decreases the use of pointless contexts, and omits redundant Enabled: true config.

• Refactor ForbidRBIOutsideOfAllowedPaths
  This refactors the cop to inherit from RuboCop::Cop::Base instead of the deprecated RuboCop::Cop::Cop class.

• Assumes ForbidRBIOutsideOfAllowedPaths config is valid
  Idiomatically, cops do not validate their config. Instead, they provide good defaults, and trust the host application to follow their example.

  The existing approach of adding an offense on every file is not a particularly good experience either, so we can just keep it simple and remove it.

• Adds global offenses in ForbidRBIOutsideOfAllowedPaths
  The offense is with regards to the file name, so a global offense is more appropriate.

• Renames ForbidRBIOutsideOfAllowedPaths to RBIFilePath
  This name is more idiomatic and focussed on what is allowed (using the allowed paths) rather than forbidding other paths.
  This is technically a breaking change, but if it applies to the host repo, RuboCop will fail to start up with an error explaining exactly what needs to be changed in the config, so it would not break anyone's production environments or corrupt any code.

⚠️ Before Merging

• Rebase after Add obsoletion config #207 is merged.

[andyw8]

What is the {} here?

[sambostock]

It is used to denote an offense attached to a blank line (or line containing only a comment).

https://github.com/rubocop/rubocop/blob/3cc5eadb6c70071049665a2e6c93ba389e17af14/lib/rubocop/rspec/expect_offense.rb#L94-L102

The previous implementation attached the offense to the first character in the file which looked like

        ^ RBI file path should match one of: rbi/**, sorbet/rbi/**

after switching to expect_offense but before switching to add_global_offense (see commit history).

After switching to add_global_offense, expect_offense expects ^{}.

[sambostock]

Incidentally, I just noticed rubocop/rubocop#12802 made the same change to some cops in RuboCop proper, and has the same changes to the tests.

[andyw8]

Looks good to me but I'd like if @KaanOzkan or @paracycle can confirm.

[sambostock]

It looks like expect_offense actually expects the filename as the optional second positional argument, not as a filename: keyword. For some reason, the tests are still passing though... 🤔

I'm going to mark this as a draft until I get that sorted. It's weird that the tests are passing, so I'd like to figure out why that is, so I can report the bug in expect_offense or expect_no_offenses, if one exists.

[paracycle]

The changes look good to me, but it would be great to understand what's happening with the tests.

[paracycle]

We might need a VersionChanged: entry here.

[sambostock]

Okay, so it took some digging, but I think I figured out what was going on with the tests.

Nothing complains about receiving a Hash as the file, instead of nil, a String, or a File object (I found some paths that look for respond_to?(:write), so I assume that's what that's for), so it makes it all the way into the ProcessedSource object, which hands it off to a Parser::Source::Buffer, which converts it to a String ("{filename: '...'}").

From there, when RuboCop::Cop::Team#roundup_relevant_cops gathers the on_duty cops which will be iterated through to investigate the source and find offenses, it excludes cops if cop.excluded_file?(processed_source.file_path). Because this cop is configured to only Include: ['**/*.rbi'], and the file_path is now "{filename: '...'}", it doesn't match, so the cop is skipped, and there are no offenses.

RuboCop itself is "immune" to this bug because InternalAffairs/RedundantExpectOffenseArguments catches when keyword arguments are passed to expect_offense.

I'll look into enabling RedundantExpectOffenseArguments in this codebase, as well as if we can add a type check in expect_no_offenses upstream.

[sambostock]

Ahh, it looks like the reason that cop didn't trigger is because we're on an outdated RuboCop version, and it was only added a month ago. I'll look at adding Dependabot to this repo, which should prevent that going forward.

[github-actions]

This PR has been automatically marked as stale because it has not had recent activity. It will be closed if no further activity occurs. Thank you for your contributions.