Skip to content

Let RSpec/SpecFilePathFormat leverage ActiveSupport inflections when configured #2090

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.

Sign up for GitHub

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

Open
wants to merge 1 commit into
base: master
Choose a base branch
from
Open

Let RSpec/SpecFilePathFormat leverage ActiveSupport inflections when configured #2090

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
1 change: 1 addition & 0 deletions CHANGELOG.md
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -3,6 +3,7 @@
## Master (Unreleased)

- Mark `RSpec/IncludeExamples` as `SafeAutoCorrect: false`. ([@yujideveloper])
- Let `RSpec/SpecFilePathFormat` leverage ActiveSupport inflections when configured. ([@corsonknowles])

## 3.6.0 (2025-04-18)

Expand Down
2 changes: 2 additions & 0 deletions config/default.yml
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -926,6 +926,8 @@ RSpec/SpecFilePathFormat:
IgnoreMethods: false
IgnoreMetadata:
type: routing
InflectorPath: "./config/initializers/inflections.rb"
Copy link
Contributor Author

Choose a reason for hiding this comment

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

most should never need to set this, but just in case

UseActiveSupportInflections: false
Copy link
Collaborator

@bquorning bquorning Jun 8, 2025 •
edited
Loading

Choose a reason for hiding this comment

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

I am not saying that we should support more inflectors, but just in case: Should we rename this to e.g. Inflector: (default|active_support)? (And a SupportedInflectors?) Or would that be premature?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I had tried to explain this choice in a PR comment:

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.

Happy to change it if you want!

But to reframe the above, I was on the "premature" side as I was implementing this. I thought it would be more user-friendly to use a boolean than have folks look up two "magic strings" to use this. They need both the key and the value if we abstract it, whereas they just need the key if it's a boolean.

Just let me know if you want it though.

VersionAdded: '2.24'
Reference: https://www.rubydoc.info/gems/rubocop-rspec/RuboCop/Cop/RSpec/SpecFilePathFormat

Expand Down
19 changes: 19 additions & 0 deletions docs/modules/ROOT/pages/cops_rspec.adoc
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -5951,6 +5951,17 @@ my_class_spec.rb # describe MyClass, '#method'
whatever_spec.rb # describe MyClass, type: :routing do; end
----

[#_useactivesupportinflections_-true_-rspecspecfilepathformat]
==== `UseActiveSupportInflections: true`

[source,ruby]
----
# Enable to use ActiveSupport's inflector for custom acronyms
# like HTTP, etc. Set to false by default.
# The InflectorPath provides the path to the inflector file.
# The default is ./config/initializers/inflections.rb.
----

[#configurable-attributes-rspecspecfilepathformat]
=== Configurable attributes

Expand All @@ -5976,6 +5987,14 @@ whatever_spec.rb # describe MyClass, type: :routing do; end
| IgnoreMetadata
| `{"type" => "routing"}`
|

| InflectorPath
| `./config/initializers/inflections.rb`
| String

| UseActiveSupportInflections
| `false`
| Boolean
|===

[#references-rspecspecfilepathformat]
Expand Down
70 changes: 66 additions & 4 deletions lib/rubocop/cop/rspec/spec_file_path_format.rb
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -32,6 +32,12 @@ module RSpec
# # good
# whatever_spec.rb # describe MyClass, type: :routing do; end
#
# @example `UseActiveSupportInflections: true`
# # Enable to use ActiveSupport's inflector for custom acronyms
# # like HTTP, etc. Set to false by default.
# # The InflectorPath provides the path to the inflector file.
# # The default is ./config/initializers/inflections.rb.
#
class SpecFilePathFormat < Base
include TopLevelGroup
include Namespace
Expand All @@ -57,8 +63,67 @@ def on_top_level_example_group(node)
end
end

# For testing and debugging
def self.reset_activesupport_cache!
Copy link
Collaborator

Choose a reason for hiding this comment

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

I am not crazy about this method. It feels like maybe we are memoizing on the wrong level somehow.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I kind of thought we should hoist this (the logic is in the module still), so we can use it more transparently.

But open to alternatives...

ActiveSupportInflector.reset_cache!
end

private

# Inflector module that uses ActiveSupport for advanced inflection rules
module ActiveSupportInflector
def self.call(string)
ActiveSupport::Inflector.underscore(string)
end

def self.available?(cop_config)
Copy link
Collaborator

Choose a reason for hiding this comment

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

I am not sure if this method should memoize without considering cop_config.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I think I need the config, but I will see what I can do

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I mean even if I separate the availability from the config check, I still need the config to look up the inflector_path and only in the case where it is available? or something like it.

So config is somewhat coupled to availability

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Best I can think of would be something like this, but that seems worse to me:

        def active_support_available?
          return false unless cop_config.fetch(
            'UseActiveSupportInflections',
            false
          )

          ActiveSupportInflector.available?(cop_config)
        end

        def inflector
          @inflector ||= if active_support_available?
                           ActiveSupportInflector
                         else
                           DefaultInflector
                         end
        end

Let me know whart you're aiming for here

Copy link
Collaborator

Choose a reason for hiding this comment

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

Thinking this though once more, I happened to ask myself this question: Do we expect people to give different values for InflectorPath in different configuration files? Would that even work? I think ActiveSupport::Inflector sets its inflections globally.

So, should InflectorPath really be a global configuration that cannot be overwritten?

Copy link
Contributor Author

@corsonknowles corsonknowles Jun 14, 2025 •
edited
Loading

Choose a reason for hiding this comment

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

I would guess the thinking here is that someone using ActiveSupport but not Rails might have a customized directory structure. And monkey patchers could still use this.

ActiveSupport::Inflector does seem pretty locked down against changes to the base file, and I imagine the same reasoning applies to not moving the config file around, so Rails itself should be pretty stable around which path it scaffolds the file to.

Maybe something else with nested Rails apps or nested Rails Engines?

I do think this is mildly superfluous, but it was one of the comment requests on the original PR, and now that we have it, I'm hesitant to rip it out again since in theory we're making life easier for +1% of users.

IMO, the heavyweight thing is we have to require 2 files and fail gracefully. Path configurability is minor compared that that.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

If you think its worth the sacrifice, we could take the path out of config and then make it a constant on the cop class.

I'm imagining somewhere someone will eventually have to type ruby -e and patch our cop as they run it from the command line to get it working for their use case, but perhaps the tradeoff is worth it?

return @available unless @available.nil?

unless cop_config.fetch('UseActiveSupportInflections', false)
return @available = false
end

unless File.exist?(inflector_path(cop_config))
return @available = false
end

@available = begin
require 'active_support/inflector'
require inflector_path(cop_config)
true
rescue LoadError, StandardError
false
end
end

def self.inflector_path(cop_config)
cop_config.fetch('InflectorPath',
'./config/initializers/inflections.rb')
end

def self.reset_cache!
@available = nil
end
end

# Inflector module that uses basic regex-based conversion
module DefaultInflector
def self.call(string)
string
.gsub(/([^A-Z])([A-Z]+)/, '\1_\2')
.gsub(/([A-Z])([A-Z][^A-Z\d]+)/, '\1_\2')
.downcase
end
end

def inflector
@inflector ||= if ActiveSupportInflector.available?(cop_config)
ActiveSupportInflector
else
DefaultInflector
end
end

def ensure_correct_file_path(send_node, class_name, arguments)
pattern = correct_path_pattern(class_name, arguments)
return if filename_ends_with?(pattern)
Expand Down Expand Up @@ -106,10 +171,7 @@ def expected_path(constant)
end

def camel_to_snake_case(string)
string
.gsub(/([^A-Z])([A-Z]+)/, '\1_\2')
.gsub(/([A-Z])([A-Z][^A-Z\d]+)/, '\1_\2')
.downcase
inflector.call(string)
end

def custom_transform
Expand Down
Loading