From 640f458a8301ee19c486d747af292a32080540f2 Mon Sep 17 00:00:00 2001 From: Jon Schneider Date: Thu, 5 Mar 2020 17:39:12 -0500 Subject: [PATCH] [Fix rubocop-hq#6289] Naming/FileName: Add CheckDefinitionPathHierarchy option --- CHANGELOG.md | 5 ++ config/default.yml | 4 + lib/rubocop/cop/naming/file_name.rb | 35 +++++--- manual/cops_naming.md | 1 + spec/rubocop/cop/naming/file_name_spec.rb | 104 +++++++++++++++++++++- 5 files changed, 138 insertions(+), 11 deletions(-) diff --git a/CHANGELOG.md b/CHANGELOG.md index 191ed36494c..b8d2c0fcd32 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -2,6 +2,10 @@ ## master (unreleased) +### New features + +* [#6289](https://github.com/rubocop-hq/rubocop/issues/6289): Add new `CheckDefinitionPathHierarchy` option for `Naming/FileName`. ([@jschneid][]) + ### Bug fixes * [#8008](https://github.com/rubocop-hq/rubocop/issues/8008): Fix an error for `Lint/SuppressedException` when empty rescue block in `def`. ([@koic][]) @@ -4542,3 +4546,4 @@ [@laurmurclar]: https://github.com/laurmurclar [@jethrodaniel]: https://github.com/jethrodaniel [@CvX]: https://github.com/CvX +[@jschneid]: https://github.com/jschneid diff --git a/config/default.yml b/config/default.yml index 188a338789a..8401ac80c58 100644 --- a/config/default.yml +++ b/config/default.yml @@ -2031,6 +2031,10 @@ Naming/FileName: # It further expects it to be nested inside modules which match the names # of subdirectories in its path. ExpectMatchingDefinition: false + # When `false`, changes the behavior of ExpectMatchingDefinition to match only + # whether each source file's class or module name matches the file name -- + # not whether the nested module hierarchy matches the subdirectory path. + CheckDefinitionPathHierarchy: true # If non-`nil`, expect all source file names to match the following regex. # Only the file name itself is matched, not the entire file path. # Use anchors as necessary if you want to match the entire name rather than diff --git a/lib/rubocop/cop/naming/file_name.rb b/lib/rubocop/cop/naming/file_name.rb index e6d21bcff79..da46a5d5a97 100644 --- a/lib/rubocop/cop/naming/file_name.rb +++ b/lib/rubocop/cop/naming/file_name.rb @@ -49,25 +49,36 @@ def investigate(processed_source) def for_bad_filename(file_path) basename = File.basename(file_path) - msg = if filename_good?(basename) - return if matching_definition?(file_path) - no_definition_message(basename, file_path) - else - return if bad_filename_allowed? + if filename_good?(basename) + msg = perform_class_and_module_naming_checks(file_path, basename) + else + msg = other_message(basename) unless bad_filename_allowed? + end - other_message(basename) - end + yield source_range(processed_source.buffer, 1, 0), msg if msg + end - yield source_range(processed_source.buffer, 1, 0), msg + def perform_class_and_module_naming_checks(file_path, basename) + return unless expect_matching_definition? + + if check_definition_path_hierarchy? && + !matching_definition?(file_path) + msg = no_definition_message(basename, file_path) + elsif !matching_class?(basename) + msg = no_definition_message(basename, basename) + end + msg end def matching_definition?(file_path) - return true unless expect_matching_definition? - find_class_or_module(processed_source.ast, to_namespace(file_path)) end + def matching_class?(file_name) + find_class_or_module(processed_source.ast, to_namespace(file_name)) + end + def bad_filename_allowed? ignore_executable_scripts? && processed_source.start_with?('#!') end @@ -94,6 +105,10 @@ def expect_matching_definition? cop_config['ExpectMatchingDefinition'] end + def check_definition_path_hierarchy? + cop_config['CheckDefinitionPathHierarchy'] + end + def regex cop_config['Regex'] end diff --git a/manual/cops_naming.md b/manual/cops_naming.md index 61b06e6e56f..a640407c07d 100644 --- a/manual/cops_naming.md +++ b/manual/cops_naming.md @@ -242,6 +242,7 @@ Name | Default value | Configurable values --- | --- | --- Exclude | `[]` | Array ExpectMatchingDefinition | `false` | Boolean +CheckDefinitionPathHierarchy | `true` | Boolean Regex | `` | IgnoreExecutableScripts | `true` | Boolean AllowedAcronyms | `CLI`, `DSL`, `ACL`, `API`, `ASCII`, `CPU`, `CSS`, `DNS`, `EOF`, `GUID`, `HTML`, `HTTP`, `HTTPS`, `ID`, `IP`, `JSON`, `LHS`, `QPS`, `RAM`, `RHS`, `RPC`, `SLA`, `SMTP`, `SQL`, `SSH`, `TCP`, `TLS`, `TTL`, `UDP`, `UI`, `UID`, `UUID`, `URI`, `URL`, `UTF8`, `VM`, `XML`, `XMPP`, `XSRF`, `XSS` | Array diff --git a/spec/rubocop/cop/naming/file_name_spec.rb b/spec/rubocop/cop/naming/file_name_spec.rb index 8de392d8283..ab848a649e9 100644 --- a/spec/rubocop/cop/naming/file_name_spec.rb +++ b/spec/rubocop/cop/naming/file_name_spec.rb @@ -126,7 +126,11 @@ context 'when ExpectMatchingDefinition is true' do let(:cop_config) do - { 'IgnoreExecutableScripts' => true, 'ExpectMatchingDefinition' => true } + { + 'IgnoreExecutableScripts' => true, + 'ExpectMatchingDefinition' => true, + 'CheckDefinitionPathHierarchy' => 'true' + } end context 'on a file which defines no class or module at all' do @@ -273,6 +277,104 @@ class B end end + context 'when CheckDefinitionPathHierarchy is false' do + let(:cop_config) do + { + 'IgnoreExecutableScripts' => true, + 'ExpectMatchingDefinition' => true, + 'CheckDefinitionPathHierarchy' => false + } + end + + context 'on a file with a matching class' do + let(:source) { <<~RUBY } + begin + class ImageCollection + end + end + RUBY + let(:filename) { '/lib/image_collection.rb' } + + it 'does not register an offense' do + expect(cop.offenses.empty?).to be(true) + end + end + + context 'on a file with a non-matching class' do + let(:source) { <<~RUBY } + begin + class PictureCollection + end + end + RUBY + let(:filename) { '/lib/image_collection.rb' } + + it 'registers an offense' do + expect(cop.offenses.size).to eq(1) + expect(cop.messages).to eq(['image_collection.rb should define a ' \ + 'class or module called ' \ + '`ImageCollection`.']) + end + end + + context 'on an empty file' do + let(:source) { '' } + let(:filename) { '/lib/rubocop/foo.rb' } + + it 'registers an offense' do + expect(cop.offenses.size).to eq(1) + expect(cop.messages).to eq(['foo.rb should define a class ' \ + 'or module called `Foo`.']) + end + end + + context 'in a non-matching directory, but with a matching class' do + let(:source) { <<~RUBY } + begin + module Foo + end + end + RUBY + let(:filename) { '/lib/some/path/foo.rb' } + + it 'does not register an offense' do + expect(cop.offenses.empty?).to be(true) + end + end + + context 'with a non-matching module containing a matching class' do + let(:source) { <<~RUBY } + begin + module NonMatching + class Foo + end + end + end + RUBY + let(:filename) { 'lib/foo.rb' } + + it 'does not register an offense' do + expect(cop.offenses.empty?).to be(true) + end + end + + context 'with a matching module containing a non-matching class' do + let(:source) { <<~RUBY } + begin + module Foo + class NonMatching + end + end + end + RUBY + let(:filename) { 'lib/foo.rb' } + + it 'does not register an offense' do + expect(cop.offenses.empty?).to be(true) + end + end + end + context 'when Regex is set' do let(:cop_config) { { 'Regex' => /\A[aeiou]\z/i } }