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.

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

[Fix #6132] Fix a false negative for Naming/FileName #6145

Merged

Conversation

koic
Copy link
Member

@koic koic commented Jul 31, 2018

Fixes #6132.

Summary

This PR fixes a false negative for Naming/FileName when Include of AllCops is the default setting.

The important change in this PR is below.

diff --git a/lib/rubocop/cop/naming/file_name.rb
b/lib/rubocop/cop/naming/file_name.rb
index a025a73b7..be3f1c5aa 100644
--- a/lib/rubocop/cop/naming/file_name.rb
+++ b/lib/rubocop/cop/naming/file_name.rb
@@ -32,7 +32,7 @@ module RuboCop

         def investigate(processed_source)
           file_path = processed_source.file_path
-          return if config.file_to_include?(file_path)
+          return if config.file_to_exclude?(file_path)

The problem is that the target to be excluded was Include instead of Exclude.

Also this PR adds the RuboCop::Config#allowed_camel_case_file? method to judge ignoring Gemfile, Rakefile, etc described in Include.

Other Information

This false negative was noticed by adding **/*.rb to config/default.yml at #5882.

https://github.com/rubocop-hq/rubocop/pull/5882/files#diff-e93280b3b31a6438c533a5f3232340d8R18


Before submitting the PR make sure the following are checked:

  • Wrote good commit messages.
  • Commit message starts with [Fix #issue-number] (if the related issue exists).
  • Feature branch is up-to-date with master (if not - rebase it).
  • Squashed related commits together.
  • Added tests.
  • Added an entry to the Changelog if the new code introduces user-observable changes. See changelog entry format.
  • The PR relates to only one subject with a clear title
    and description in grammatically correct, complete sentences.
  • Run rake default or rake parallel. It executes all tests and RuboCop for itself, and generates the documentation.

@koic koic force-pushed the fix_false_nagative_for_naming_file_name branch 3 times, most recently from f6bb55e to 507a33e Compare August 1, 2018 01:13

def allowed_camel_case_file?(file)
file_to_include?(file) do |pattern, relative_path, absolute_path|
if pattern.to_s =~ /[A-Z]/
Copy link
Contributor

Choose a reason for hiding this comment

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

I think this if statement can be converted to

pattern.to_s =~ /[A-Z]/ &&
  (match_path?(pattern, relative_path) ||
   match_path?(pattern, absolute_path))

That way the else can be eliminated entirely.

Also, what kind of object is pattern? Assuming that pattern is a symbol, to_s is unnecessary because =~ works with symbols.

Copy link
Member Author

Choose a reason for hiding this comment

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

That way the else can be eliminated entirely.

Oh, I see! I fixed the conditional expression simple.

Also, what kind of object is pattern?

If !ruby/regexp /regexp$/ is written in Include of .rubocop.yml, a Regexp instance is returned.
https://github.com/rubocop-hq/rubocop/blob/1bff66b2b8bafd215c106ee06411277d5554581d/spec/rubocop/cli_spec.rb#L996

In that case this code need to_s to prevent TypeError.

Copy link
Contributor

Choose a reason for hiding this comment

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

If !ruby/regexp /regexp$/ is written in Include of .rubocop.yml, a Regexp instance is returned.

That makes more sense. I wasn't even thinking about pattern being regex. I'm so used to the paradigm of to_s =~ // being used unnecessarily with symbols.

Fixes rubocop#6132.

### Summary

This PR fixes a false negative for `Naming/FileName`
when `Include` of `AllCops` is the default setting.

The important change in this PR is below.

```diff
diff --git a/lib/rubocop/cop/naming/file_name.rb
b/lib/rubocop/cop/naming/file_name.rb
index a025a73b7..be3f1c5aa 100644
--- a/lib/rubocop/cop/naming/file_name.rb
+++ b/lib/rubocop/cop/naming/file_name.rb
@@ -32,7 +32,7 @@ module RuboCop

         def investigate(processed_source)
           file_path = processed_source.file_path
-          return if config.file_to_include?(file_path)
+          return if config.file_to_exclude?(file_path)
```

The problem is that the target to be excluded was
`Include` instead of `Exclude`.

Also this PR adds the `RuboCop::Config#allowed_camel_case_file?`
method to judge ignoring `Gemfile`, `Rakefile`, etc
described in` Include`.

### Other Information

This false negative was noticed by adding `**/*.rb` to
config/default.yml at rubocop#5882.

https://github.com/rubocop-hq/rubocop/pull/5882/files#diff-e93280b3b31a6438c533a5f3232340d8R18
@koic koic force-pushed the fix_false_nagative_for_naming_file_name branch from 507a33e to 28d3daf Compare August 2, 2018 10:51
@bbatsov bbatsov merged commit 514131b into rubocop:master Aug 3, 2018
@koic koic deleted the fix_false_nagative_for_naming_file_name branch August 3, 2018 08:48
koic added a commit to koic/rubocop that referenced this pull request Aug 3, 2018
Follow up of rubocop#6145.

This is the above overlook.

This PR updates the configuration comment to the behavior
of `Naming/FileName` that changed with rubocop#6145.
bbatsov pushed a commit that referenced this pull request Aug 4, 2018
Follow up of #6145.

This is the above overlook.

This PR updates the configuration comment to the behavior
of `Naming/FileName` that changed with #6145.
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.

Naming/FileName Cop not working
3 participants