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

[#494] Create (or use) a Rubocop for the inverse keyword needed in the associations #495

Merged
merged 10 commits into from
Jan 11, 2024

Conversation

Goose97
Copy link
Contributor

@Goose97 Goose97 commented Jan 3, 2024

What happened 👀

Implement custom cop to enforce explicit inverse_of when specifying Rails association
Compass rule

Insight 📝

  • Detect Rails model files and only run this cop on such files
  • Run test on custom cops during CI (I think we should do it on another ticket)

There are severals trade-offs that I made to reduce the complexity of the cop:

  • We can improve the performance by only scanning the top level expressions of the class. I haven't tested it yet, but I suspect that the gain would be negligible. The reason is that we use a lot of different cop rules so that there's a good chance that we have to scan the whole file anyway.
  • There's no easy way to distinguish between Rails association helpers and our application functions. If the names collide, it might lead to false positive. However, in practice, we should avoid from naming our methods this way, so I think we could safely ignore this case.
class Author < ActiveRecord::Model
  has_many :books # we should scan this one
  belongs_to :user # and this one

  def test
    SomeModule.has_many(...) # but not this one
  end
end

Note

I encounter a weird bug that prevents me from copying the custom cops to lib/ folder. That's why I have to keep it in the root folder. For more information, see this

Proof Of Work 📹

Screen-Recording-2024-01-03-at-11.59.47.mp4

@Goose97 Goose97 self-assigned this Jan 3, 2024
@Goose97 Goose97 added this to the 5.12.0 milestone Jan 3, 2024
@Goose97 Goose97 force-pushed the feature/gh494-custom-rubocop-inverse-of-association branch 14 times, most recently from 194a4e0 to 4ee9b70 Compare January 5, 2024 07:52
@Goose97 Goose97 force-pushed the feature/gh494-custom-rubocop-inverse-of-association branch from 4ee9b70 to f9dc30a Compare January 5, 2024 08:17
@Goose97 Goose97 force-pushed the feature/gh494-custom-rubocop-inverse-of-association branch from bd14c0e to a384ec3 Compare January 5, 2024 08:19
@Goose97 Goose97 marked this pull request as ready for review January 5, 2024 08:38
@Goose97 Goose97 requested a review from a user January 5, 2024 08:38
@Goose97 Goose97 requested a review from sanG-github as a code owner January 5, 2024 08:38
@Goose97
Copy link
Contributor Author

Goose97 commented Jan 8, 2024

@malparty Now think about it, I think we better off splitting the custom cops into a separate repo 🤔 . It's likely that we gonna have more custom cops coming in the future and also makes it easier for current project to adopt the new cops. What do you think?

Copy link
Member

@malparty malparty left a comment

Choose a reason for hiding this comment

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

Thanks for the clear insights about the arbitrations made. That seems fair enough to me 💡

Approved ahead with a question/suggestion 👍

.rubocop.yml Outdated
@@ -2,6 +2,7 @@ require:
- rubocop-rails
- rubocop-rspec
- rubocop-performance
- ./custom_cops/required_inverse_of_relations.rb
Copy link
Member

Choose a reason for hiding this comment

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

I'm not sure if that works, but can we try to rename the folder: ./rubocop/cops/. At first glance, custom_cops might be hard to recognize for people new in the world of Rails 🛤️

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Agree. Fixed in fba261b

@Goose97 Goose97 force-pushed the feature/gh494-custom-rubocop-inverse-of-association branch from 3457045 to fba261b Compare January 9, 2024 09:54
Copy link
Contributor

@sanG-github sanG-github left a comment

Choose a reason for hiding this comment

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

What a delightful change!! 🎉

rubocop/Gemfile Show resolved Hide resolved
.rubocop.yml Outdated Show resolved Hide resolved
rubocop/cops/required_inverse_of_relations.rb Outdated Show resolved Hide resolved
@malparty malparty added this pull request to the merge queue Jan 11, 2024
Merged via the queue into develop with commit ccf3752 Jan 11, 2024
5 checks passed
@malparty malparty deleted the feature/gh494-custom-rubocop-inverse-of-association branch January 11, 2024 01:03
This was referenced Jan 11, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Create (or use) a Rubocop for the inverse keyword needed in the associations
3 participants