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

[#10221] Update Naming/FileName to recognize Structs as classes that satisfy the ExpectMatchingDefinition requirement #10223

Merged
merged 3 commits into from Nov 12, 2021

Conversation

dvandersluis
Copy link
Member

@dvandersluis dvandersluis commented Oct 29, 2021

Allows Structs assigned to constants to be used for ExpectMatchingDefinition of Naming::FileName.

Allow refactors the cop's tests to use expect_offense and adds some documentation for options that aren't explained.

Fixes #10221.


Before submitting the PR make sure the following are checked:

  • The PR relates to only one subject with a clear title and description in grammatically correct, complete sentences.
  • 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.
  • Ran bundle exec rake default. It executes all tests and runs RuboCop on its own code.
  • Added an entry (file) to the changelog folder named {change_type}_{change_description}.md if the new code introduces user-observable changes. See changelog entry format for details.

@splattael
Copy link
Contributor

@dvandersluis Thanks for opening this PR so quickly ❤️

I think this is a good start but I was wondering if we need to take care of non-Struct related assignments as well to support e.g. aliasing:

module Something
  Bar = ::OtherNamespace::Bar
end

At GitLab we use such technique for aliasing in particular, for example: https://gitlab.com/gitlab-org/gitlab/-/blob/4e75a27af3add26b45c4dd05314946757a670578/ee/lib/elastic/v12p1/routing.rb#L6

@dvandersluis
Copy link
Member Author

I'm not sold on having a constant defined satisfying the FileName cop. For Struct it's one thing because it's actually defining a class, but a constant could be anything.

Would you think this should satisfy the ExpectMatchingDefinition requirement?

module Foo
  Bar = 5
end

For your specific use case, I wonder if just excluding gitlab/ee/lib/elastic/v12p1/**/* from this cop would be a better solution.

@splattael
Copy link
Contributor

I'm not sold on having a constant defined satisfying the FileName cop. For Struct it's one thing because it's actually defining a class, but a constant could be anything.

Would you think this should satisfy the ExpectMatchingDefinition requirement?

module Foo
  Bar = 5
end

Yes, I think this cop should only check whether the defined constant matches the file name. The value of the constant can be anything - which is zeitwerk's approach as well.

Note that Ruby has many ways of creating a class/module so it don't have to be Struct specific at all:

module Foo
  Bar = SomeClassFactory.create
end 

See for example, https://github.com/dry-rb/dry-types/blob/9562995e50f9dfba0b8628224b07d865d18335da/lib/dry/types/any.rb#L45.

For your specific use case, I wonder if just excluding gitlab/ee/lib/elastic/v12p1/**/* from this cop would be a better solution.

We did exclude them for now but ideally we'd also support aliasing hence the issue 😅

@bbatsov bbatsov merged commit 4e49d27 into rubocop:master Nov 12, 2021
@dvandersluis dvandersluis changed the title [#10221] Update Naming::FileName to recognize Structs as classes that satisfy the ExpectMatchingDefinition requirement [#10221] Update Naming/FileName to recognize Structs as classes that satisfy the ExpectMatchingDefinition requirement Nov 15, 2021
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: Detect constant assignment
3 participants