[Fix #6289] Naming/FileName: Add ExpectMatchingClass option

[jschneid]

#6289 describes a limitation of the existing ExpectMatchingDefinition: true option on the Naming/FileName cop: It generates false positives when run on a project with classes which (by design) aren't namespaced with modules to match their file paths.

This PR addresses that issue by adding a new option to the Naming/FileName cop: CheckDefinitionPathHierarchy. This defaults to true; if set to false, it modifies the ExpectMatchingDefinition option to only check whether each source file's class or module name matches the file name -- not whether the nested module hierarchy matches the subdirectory path.

---

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 bundle exec rake default. It executes all tests and RuboCop for itself, and generates the documentation.

[bbatsov]

If RuboCop is configured to enable both the ExpectMatchingDefinition and ExpectMatchingClass options, then the ExpectMatchingDefinition option (which is more strict) takes precedence.

I think it's much better to just add an option for checking the path hierarchy instead, as it seems the new option you introduced is the same as the other sans the path checking. For me the suggested solution is confusing, but valuable - let's simplify it. :-)

[jschneid]

I think it's much better to just add an option for checking the path hierarchy instead, as it seems the new option you introduced is the same as the other sans the path checking. For me the suggested solution is confusing, but valuable - let's simplify it. :-)

Bozhidar, thanks for the comment! I'd like to fix up this PR to address your suggestion; however, I'm not entirely sure what you're suggesting -- could you please clarify?

The ExpectMatchingDefinition option on the Naming/FileName cop, as it exists today, when enabled, essentially enforces two additional checks:

1. That the file's module namespace matches its filesystem path;
2. That the name of the class or module defined in the file matches the filename.

This PR's new proposed option (ExpectMatchingClass), when enabled, checks only item 2 above.

Are you suggesting that we add an additional new option that would check only item 1 above? Such that we'd end up with Naming/FileName cop options:

• Existing option ExpectMatchingDefinition, which checks items 1 + 2 above;
• New option ExpectMatchingClass, which checks only item 2;
• Another new option -- ExpectMatchingPathHierarchy, perhaps? -- which checks only item 1?

I'd be happy to take that on -- I just want to check my understanding of whether that's what you're suggesting, first. 🙂

[jschneid]

@bbatsov, this is 100% not urgent, but I wanted to bump my above question for you, just in case you hadn't seen it yet. 🙂

[bbatsov]

Sorry about the slow reply. My bad, my previous reply was completely wrong. I was under the impression that the cop already was checking for some definition and I had assumed you wanted to limit the full path checking and focus only on checking for a definition. I saw this was not the case, so it seems everything's good with your PR. Just rebase to resolve the merge conflict, please.

[bbatsov]

Ah, forget what I said. I'm too tired for meaningful replies. Actually seems I wasn't wrong last time around! What I meant was that instead of having ExpectMatchingClass and ExpectMatchingDefinition we can have something like CheckDefinitionPathHieararchy which would alter the behaviour of ExpectMatchingDefinition. Alternatively we can make ExpectMatchingDefinition accept non-boolean values, so you can specify there something like match_hierarchy, match_filename, etc. I'm just thinking that one of those approaches will result in more understandable configuration options.

[jschneid]

Ok! As per your suggestion, @bbatsov, I've adjusted this PR to instead add a new CheckDefinitionPathHierarchy option to the Naming/FileName cop. Details are in the updated PR description, above.

[jschneid]

I've rebased this PR to fix the typical merge conflict in the CHANGELOG.md (which occurs over time as other PRs are merged).

@bbatsov, is there anything else I can do to help with this at this point? 🙂

[bbatsov]

Thanks and sorry for forgetting about this PR!