rubocop-rspec icon indicating copy to clipboard operation
rubocop-rspec copied to clipboard

Let `RSpec/SpecFilePathFormat` leverage ActiveSupport inflections when configured

Open corsonknowles opened this issue 5 months ago • 3 comments

Fix #740

Credit to this PR, reviving with performance and configuration comments addressed:

  • https://github.com/rubocop/rubocop-rspec/pull/1266/files#r862618003

Before submitting the PR make sure the following are checked:

  • [x] Feature branch is up-to-date with master (if not - rebase it).
  • [x] Squashed related commits together.
  • [x] Added tests.
  • [x] Updated documentation.
  • [x] Added an entry to the CHANGELOG.md if the new code introduces user-observable changes.
  • [x] The build (bundle exec rake) passes (be sure to run this locally, since it may produce updated documentation that you will need to commit).

If you have created a new cop:

  • [ ] Added the new cop to config/default.yml.
  • [ ] The cop is configured as Enabled: pending in config/default.yml.
  • [ ] The cop is configured as Enabled: true in .rubocop.yml.
  • [ ] The cop documents examples of good and bad code.
  • [ ] The tests assert both that bad code is reported and that good code is not reported.
  • [ ] Set VersionAdded: "<<next>>" in default/config.yml.

If you have modified an existing cop's configuration options:

  • [ ] Set VersionChanged: "<<next>>" in config/default.yml.

corsonknowles avatar Jun 02 '25 23:06 corsonknowles

bundle exec rake is failing on the base branch, unless I am missing something

edit: I was missing the need to bundle update to get a clean run

corsonknowles avatar Jun 02 '25 23:06 corsonknowles

Alternatively: We could remove all the handling to cache the lookup by rescuing and warning on a load error when the user has configured to use Inflectors but they aren't available.

The current approach fails gracefully when configured on, but unavailable.

corsonknowles avatar Jun 03 '25 00:06 corsonknowles

@bquorning I think I addressed all the comments on the original proposed fix PR.

We tested this locally in one of our repos with good results.

I made the path configurable, but I didn't think an array of possible integrations in config was warranted at this point.

In the event that someone does come along and adds an adapter to a new Inflector library, they could add a new top level config key and use the same InflectorPath key.

I guess I just think it's unlikely we'll reach more than 3 custom inflectors at which point we could always switch to making users pick a style instead of having a boolean option.

corsonknowles avatar Jun 03 '25 23:06 corsonknowles

@bquorning I'm not really sure where to take this. Should I remove the file path config? I added it because it was asked for in the previous PR. Also, should we leverage this to make it more of a seamless default?

https://github.com/search?q=repo%3Arubocop%2Frubocop%20ActiveSupportExtensionsEnabled&type=code

corsonknowles avatar Sep 02 '25 20:09 corsonknowles

Hi @corsonknowles, I’m sorry for being unresponsive on this issue for (checks dates) a couple of months 😮

  1. I think the memoization of @available in ActiveSupportInflector needs to be based on the cop_config, so that if people have two different .rubocop.yml files in different directories, with different configuration of this cop, we will actually use different inflectors. That means the memoization of @inflector needs to be removed. And @available needs to be stored as a Hash, with cop_config (or cop_config.hash as the key).
  2. Can we remove the .reset_activesupport_cache! and .reset_cache! methods, and instead do the resetting entirely from the spec file? Something like this:
    def reset_activesupport_cache!
      described_class::ActiveSupportInflector.instance_variable_set(
        :@available, nil # Or `{}`, as per point 1 above.
      )
    end
    
    around do |example|
      reset_activesupport_cache!
      example.run
      reset_activesupport_cache!
    end
    
  3. I’m still undecided on whether to keep UseActiveSupportInflections: true or have an Inflector: active_support instead. Perhaps @pirj has an opinion (see previous discussion). If we add a 2nd custom inflector, we’d need to decide if we want both UseActiveSupportInflections and UseDryInflections in the config, and what happens if they are both configured as true.

bquorning avatar Sep 14 '25 20:09 bquorning

Regarding ActiveSupportExtensionsEnabled, I don’t think I have used that configuration option before. It looks like it is intended to tell RuboCop to allow method like many? {} and present? on a few cops. I don’t think it is intended for things like our use case.

bquorning avatar Sep 14 '25 20:09 bquorning

@corsonknowles If you want, I can take a stab at implementing those changes I suggested above?

bquorning avatar Oct 08 '25 11:10 bquorning

Oh that would be great! Thank you!!

On Wed, Oct 8, 2025 at 4:14 AM Benjamin Quorning @.***> wrote:

bquorning left a comment (rubocop/rubocop-rspec#2090) https://github.com/rubocop/rubocop-rspec/pull/2090#issuecomment-3381028680

@corsonknowles https://github.com/corsonknowles If you want, I can take a stab at implementing those changes I suggested above?

— Reply to this email directly, view it on GitHub https://github.com/rubocop/rubocop-rspec/pull/2090#issuecomment-3381028680, or unsubscribe https://github.com/notifications/unsubscribe-auth/AANFDSY7FMD625HAZK2E2S33WTWZFAVCNFSM6AAAAAB6OHU36WVHI2DSMVQWIX3LMV43OSLTON2WKQ3PNVWWK3TUHMZTGOBRGAZDQNRYGA . You are receiving this because you were mentioned.Message ID: @.***>

corsonknowles avatar Oct 08 '25 15:10 corsonknowles

@corsonknowles, I managed to push a commit to your branch. It’s not polished yet, but feel welcome to take a look already.

bquorning avatar Oct 20 '25 12:10 bquorning

@pirj Can you help with the failing RSpec 4 tests?

bquorning avatar Oct 20 '25 12:10 bquorning

We can remove that RSpec 4 job. It’s about to be released.

pirj avatar Oct 20 '25 14:10 pirj

All the more reason to get the tests passing 🙈

bquorning avatar Oct 20 '25 17:10 bquorning

Fair enough. Seems that it’s failing because RSpec switched to having verified part doubles by default. It is puzzling howe build was passing as this has been in the build long enough.

Anyway, we seem to be stubbing underscore of AS::Inflector that doesn’t exist as we just stub it, and don’t require the gem in our build.

pirj avatar Oct 20 '25 20:10 pirj

This looks good to me. I'm going to squash and force push, as I think that's required by the repo git standards

corsonknowles avatar Nov 03 '25 18:11 corsonknowles

@bquorning Okay, I think it meets standards now. bundle exec rake passes locally and CI was green on the same changes.

corsonknowles avatar Nov 03 '25 19:11 corsonknowles

@pirj Would you have time to do the final review of this PR?

bquorning avatar Nov 03 '25 21:11 bquorning

Thank you!

pirj avatar Nov 04 '25 17:11 pirj