Skip to content

Fix a false negative for FactoryBot/AssociationStyle when using keyword traits with explicit associations#168

Open
r7kamura wants to merge 1 commit intorubocop:masterfrom
r7kamura:AssociationStyle-keyword-trait
Open

Fix a false negative for FactoryBot/AssociationStyle when using keyword traits with explicit associations#168
r7kamura wants to merge 1 commit intorubocop:masterfrom
r7kamura:AssociationStyle-keyword-trait

Conversation

@r7kamura
Copy link
Contributor

@r7kamura r7kamura commented Nov 18, 2025

A change related to keywords was introduced here:

At that time, the implementation was adjusted so that the following code would not be treated as an offense, but shouldn’t this actually be considered an offense?

# EnforcedStyle: implicit

# bad
association :foo, :alias

# good
foo factory: %i[foo alias]

In addition to changing the implementation, I made a few improvements:

  1. Converted KEYWORDS from an Array to a Set to improve performance.
  2. Updated the code so that only correctable explicit associations are treated as offenses, making the intention clearer.

Before submitting the PR make sure the following are checked:

  • Feature branch is up-to-date with master (if not - rebase it).
  • Squashed related commits together.
  • Added tests.
  • Updated documentation.
  • Added an entry to the CHANGELOG.md if the new code introduces user-observable changes.
  • The build (bundle exec rake) passes (be sure to run this locally, since it may produce updated documentation that you will need to commit).

If you have created a new cop:

  • Added the new cop to config/default.yml.
  • The cop is configured as Enabled: pending in config/default.yml.
  • The cop documents examples of good and bad code.
  • The tests assert both that bad code is reported and that good code is not reported.
  • Set VersionAdded: "<<next>>" in default/config.yml.

If you have modified an existing cop's configuration options:

  • Set VersionChanged: "<<next>>" in config/default.yml.

@r7kamura r7kamura requested a review from a team as a code owner November 18, 2025 04:53
@pirj
Copy link
Member

pirj commented Nov 23, 2025

Will this combine with the other two? #167 #169
Maybe put them all in a single PR to avoid merges? I personally don't mind reviewing bigger PRs, especially when they address neighbouring issues.

@r7kamura
Copy link
Contributor Author

r7kamura commented Nov 23, 2025

I don't think these should be combined into a single pull request.

While it's true that they modify nearby lines of the same cop, increasing the likelihood of conflicts, these three changes fundamentally address different issues and each involves a modification to the cop's behavior.

There's a possibility that users may be affected by each change, and they would likely check this in CHANGELOG.md. In that case, it would be preferable for them to be able to see only the small pull requests relevant to them.

If any conflicts arise, I will resolve them as needed, and I don't mind doing that amount of work at all.

(Even so, if the opinion from the maintainer's side is that merging such finely divided pull requests is too difficult, I will consolidate them, so please let me know.)

Copy link
Member

@ydah ydah left a comment

Choose a reason for hiding this comment

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

Thank you. Looks good.

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.

3 participants