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.

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

Jump to bottom

Make RSpec/FilePath support ActiveSupport inflections, if defined #1266

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

Make RSpec/FilePath support ActiveSupport inflections, if defined #1266

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.
Jump to
Jump to file
Failed to load files.
Diff view
Diff view
2 changes: 2 additions & 0 deletions CHANGELOG.md
View file
Open in desktop
Expand Up @@ -4,6 +4,7 @@

* Drop Ruby 2.5 support. ([@ydah][])
* Add new `RSpec/ChangeByZero` cop. ([@ydah][])
* Make `RSpec/FilePath` support ActiveSupport inflections, if defined. ([@jeromedalbert][])

## 2.10.0 (2022-04-19)

Expand Down Expand Up @@ -684,3 +685,4 @@ Compatibility release so users can upgrade RuboCop to 0.51.0. No new features.
[@oshiro3]: https://github.com/oshiro3
[@ydah]: https://github.com/ydah
[@t3h2mas]: https://github.com/t3h2mas
[@jeromedalbert]: https://github.com/jeromedalbert
7 changes: 7 additions & 0 deletions lib/rubocop/cop/rspec/file_path.rb
View file
Open in desktop
Expand Up @@ -130,6 +130,13 @@ def expected_path(constant)
end

def camel_to_snake_case(string)
if defined?(ActiveSupport::Inflector)
Copy link

Choose a reason for hiding this comment

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

There are other libraries that can be used to set inflectors:

I would suggest providing a script field in the YAML files that could be run such that String#underscore could be defined if desired.

Rspec/FilePath:
  LoadInfector: |-
    require 'active_support/inflector'
    require './config/initializers/inflections'

Alternatively, we could add a field for the Inflector gem and/or definition file:

Rspec/FilePath:
  UseInflectorGem: ActiveSupport
  InflectorDefinition: ./config/initializers/inflections.rb

Doing either of these should then allow if string.respond_to?(:underscore) to be used instead of hard-coding a library and a file location.

Copy link

@cdhagmann cdhagmann Jun 29, 2022 •
edited

Choose a reason for hiding this comment

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

Another alternative may simply be to make sure the file name matches the locations in which the class is defined, regardless of inflection.

Example:

app/services/my_html_module/xml_reader.rb 
module MyHTMLModule
  class XMLReader
spec/services/my_html_module/xml_reader_spec.rb 
RSpec.describe MyHTMLModule::XMLReader do
end

This could sidestep the need for inflection.

if File.exist?('./config/initializers/inflections.rb')
require './config/initializers/inflections'
Comment on lines +134 to +135
Copy link
Collaborator

Choose a reason for hiding this comment

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

It feels like this file path should be configurable.

end
Comment on lines +134 to +136
Copy link
Author

Choose a reason for hiding this comment

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

Inflection configuration must be loaded before ActiveSupport::Inflector.underscore is executed, to take acronyms and other inflection rules into account.

That configuration is typically found in the config/initializers/inflections.rb file that is generated when creating a Rails app.

Comment on lines +133 to +136
Copy link
Collaborator

Choose a reason for hiding this comment

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

I’m afraid this code is a bit slow. Could we do the defined?/File.exists?/require thing just once and store the result perhaps a class variable?

return ActiveSupport::Inflector.underscore(string)
end

string
.gsub(/([^A-Z])([A-Z]+)/, '\1_\2')
.gsub(/([A-Z])([A-Z][^A-Z\d]+)/, '\1_\2')
Expand Down
1 change: 1 addition & 0 deletions rubocop-rspec.gemspec
View file
Open in desktop
Expand Up @@ -39,6 +39,7 @@ Gem::Specification.new do |spec|

spec.add_runtime_dependency 'rubocop', '~> 1.19'

spec.add_development_dependency 'activesupport'
spec.add_development_dependency 'rack'
spec.add_development_dependency 'rake'
spec.add_development_dependency 'rspec', '>= 3.4'
Expand Down
21 changes: 21 additions & 0 deletions spec/rubocop/cop/rspec/file_path_spec.rb
View file
Open in desktop
Expand Up @@ -246,6 +246,27 @@ class Foo
end
end

context 'when ActiveSupport Inflector is defined', order: :defined do
before { require 'active_support/inflector' }

it 'registers an offense for a bad path when there is no custom acronym' do
expect_offense(<<-RUBY, 'pvp_class_foo_spec.rb')
describe PvPClass, 'foo' do; end
^^^^^^^^^^^^^^^^^^^^^^^^ Spec path should end with `pv_p_class*foo*_spec.rb`.
RUBY
end

it 'does not register an offense when class name contains custom acronym' do
ActiveSupport::Inflector.inflections do |inflect|
inflect.acronym('PvP')
end
Comment on lines +260 to +262
Copy link
Collaborator

Choose a reason for hiding this comment

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

I wonder if we couldn’t do

ensure
  ActiveSupport::Inflector.inflections.clear(:acronyms)

instead of having the order :defined on the context block.


expect_no_offenses(<<-RUBY, 'pvp_class_foo_spec.rb')
describe PvPClass, 'foo' do; end
RUBY
end
end

context 'when configured with IgnoreMethods' do
let(:cop_config) { { 'IgnoreMethods' => true } }

Expand Down