Skip to content

Comments

Let RSpec/SpecFilePathFormat leverage ActiveSupport inflections when configured#2090

Merged
pirj merged 1 commit intorubocop:masterfrom
corsonknowles:fix_spec_file_path_format_for_inflections
Nov 4, 2025
Merged

Let RSpec/SpecFilePathFormat leverage ActiveSupport inflections when configured#2090
pirj merged 1 commit intorubocop:masterfrom
corsonknowles:fix_spec_file_path_format_for_inflections

Conversation

@corsonknowles
Copy link
Contributor

@corsonknowles corsonknowles commented Jun 2, 2025

Fix #740

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


Before submitting the PR make sure the following are checked:

  • Feature branch is up-to-date with master (if not - rebase it).
  • Squashed related commits together.
  • Added tests.
  • Updated documentation.
  • Added an entry to the CHANGELOG.md if the new code introduces user-observable changes.
  • 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 corsonknowles changed the title Let RSpec/SpecFilePathFormat leverage ActiveSupport inflections, if… Let RSpec/SpecFilePathFormat leverage ActiveSupport inflections, if defined Jun 2, 2025
@corsonknowles corsonknowles force-pushed the fix_spec_file_path_format_for_inflections branch 2 times, most recently from 6899764 to b403e05 Compare June 2, 2025 23:53
@corsonknowles
Copy link
Contributor Author

corsonknowles commented Jun 2, 2025

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 corsonknowles force-pushed the fix_spec_file_path_format_for_inflections branch 2 times, most recently from 0d66eec to 11f3e2a Compare June 3, 2025 00:05
@corsonknowles corsonknowles changed the title Let RSpec/SpecFilePathFormat leverage ActiveSupport inflections, if defined Let RSpec/SpecFilePathFormat leverage ActiveSupport inflections, if configured and defined Jun 3, 2025
@corsonknowles corsonknowles changed the title Let RSpec/SpecFilePathFormat leverage ActiveSupport inflections, if configured and defined Let RSpec/SpecFilePathFormat leverage ActiveSupport inflections when configured and defined Jun 3, 2025
@corsonknowles
Copy link
Contributor Author

corsonknowles commented Jun 3, 2025

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 corsonknowles force-pushed the fix_spec_file_path_format_for_inflections branch 2 times, most recently from 922fa1e to 49b0b1c Compare June 3, 2025 10:24
@corsonknowles corsonknowles changed the title Let RSpec/SpecFilePathFormat leverage ActiveSupport inflections when configured and defined Let RSpec/SpecFilePathFormat leverage ActiveSupport inflections when configured Jun 3, 2025
@corsonknowles corsonknowles force-pushed the fix_spec_file_path_format_for_inflections branch 6 times, most recently from 89d6175 to b798556 Compare June 3, 2025 12:04
@corsonknowles corsonknowles force-pushed the fix_spec_file_path_format_for_inflections branch from b798556 to 2a70680 Compare June 3, 2025 16:53
@corsonknowles corsonknowles marked this pull request as ready for review June 3, 2025 18:40
@corsonknowles corsonknowles requested a review from a team as a code owner June 3, 2025 18:40
@corsonknowles corsonknowles force-pushed the fix_spec_file_path_format_for_inflections branch from 2a70680 to eb47846 Compare June 3, 2025 22:53
@corsonknowles
Copy link
Contributor Author

corsonknowles commented Jun 3, 2025

@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 corsonknowles force-pushed the fix_spec_file_path_format_for_inflections branch 3 times, most recently from bd33b0f to 84381d1 Compare June 5, 2025 06:58
@corsonknowles corsonknowles force-pushed the fix_spec_file_path_format_for_inflections branch 5 times, most recently from c987a4a to 82090d3 Compare June 8, 2025 23:44
@corsonknowles corsonknowles force-pushed the fix_spec_file_path_format_for_inflections branch from 82090d3 to 0a308f6 Compare July 9, 2025 16:21
@corsonknowles
Copy link
Contributor Author

@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

@bquorning
Copy link
Collaborator

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
Copy link
Collaborator

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
Copy link
Collaborator

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

@corsonknowles
Copy link
Contributor Author

corsonknowles commented Oct 8, 2025 via email

@bquorning bquorning force-pushed the fix_spec_file_path_format_for_inflections branch from bca2a89 to eb1a6b4 Compare October 20, 2025 12:11
@bquorning
Copy link
Collaborator

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

@bquorning
Copy link
Collaborator

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

@pirj
Copy link
Member

pirj commented Oct 20, 2025

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

@bquorning
Copy link
Collaborator

All the more reason to get the tests passing 🙈

@pirj
Copy link
Member

pirj commented Oct 20, 2025

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.

@corsonknowles
Copy link
Contributor Author

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 corsonknowles force-pushed the fix_spec_file_path_format_for_inflections branch from eca02bb to 9fa48a5 Compare November 3, 2025 18:59
…n defined and configured

Fix rubocop#740

Co-Authored-By: Dave Corson-Knowles <david.corsonknowles@gusto.com>
Co-Authored-By: Benjamin Quorning <benjamin@quorning.net>
@corsonknowles corsonknowles force-pushed the fix_spec_file_path_format_for_inflections branch from 9fa48a5 to 2777fa2 Compare November 3, 2025 19:01
@corsonknowles
Copy link
Contributor Author

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

@bquorning
Copy link
Collaborator

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

ActiveSupport::Inflector.underscore(string)
end

def self.prepare_availability(config)
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Cop_config may differ across directories.
Does it make sense to cache the preparations?

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Corson and I talked about this a couple of times earlier. The main problem is that loaded inflections are stored globally, in ActiveSupport::Inflector::Inflections.instance. So it would be very hard to separate the configurations, even if we tried.

I think that for most use cases, people will only have one inflection configuration, so it’s not a problem. But perhaps we should document the issue for those who may run into the issue in the future?

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Totally reasonable. Even though the anxiety of having config race conditions never left me, we haven’t seen any real reports for … five tears?

def inflector
case cop_config.fetch('EnforcedInflector')
when 'active_support'
ActiveSupportInflector.prepare_availability(cop_config)
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

In addition to the excessive caching across directories, there can be another problem - multiple threads. RuboCop can run in threads? Or are those processes?

In any case, can we init the inflector once per inspection?

Or this would be too expensive if the inflection file is large?

I can think of a monorepo with many engines each with its own set of inflections

@pirj pirj merged commit 3f31059 into rubocop:master Nov 4, 2025
27 checks passed
@pirj
Copy link
Member

pirj commented Nov 4, 2025

Thank you!

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

Successfully merging this pull request may close these issues.

Proposal: Avoid repetitive definitions of CustomTransform in RSpec/FilePath cop

3 participants