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

Allow #traits_for_enum to be used in a FactoryBot factory without a block #944

Merged

Conversation

harrylewis
Copy link
Contributor

@harrylewis harrylewis commented Jun 27, 2020

This PR fixes a bug that arose in relation to FactoryBot's recent 6.0.0 version release. This release introduced a new method, #traits_for_enum, which can be used to dynamically define factory traits for an enumerated attribute.

The behaviour expected is that usage of this method (which does not take a block argument) does not produce a FactoryBot/AttributeDefinedStatically offense.

The actual behaviour is that it does produce this offense because it is assumed to be an attribute, which should be defined with a block. The fix is simply to add it to the list of pre-qualified methods.

The documentation surrounding this cop does not make any mention around what methods are allowed to be used without a block, so I didn't feel it was necessary to update the documentation.

I believe this constitutes a "user-observable" change, but am not sure. If you feel it does, I will add a commit to add an entry to the CHANGELOG.md.

This fixes #943.


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).

Comment on lines -7 to +13
ATTRIBUTE_DEFINING_METHODS = %i[factory trait transient ignore].freeze
ATTRIBUTE_DEFINING_METHODS = %i[
factory
ignore
trait
traits_for_enum
transient
].freeze
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Adding :traits_for_enum caused a Layout/LineLength offense, so I turned the horizontal list into a vertical list. I also alphabetized the list to align with what was done with the other constants below.

@harrylewis harrylewis marked this pull request as ready for review June 27, 2020 02:09
@harrylewis
Copy link
Contributor Author

One of the non-required builds failed, edge-rubocop. It doesn't look like the failure has anything to do with my changes. Given it is testing using the "edge" of Rubocop, I'm assuming it is susceptible to failures like this. Let me know if there is anything I can do to resurrect the failing test.

Copy link
Member

@pirj pirj left a comment

Choose a reason for hiding this comment

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

Thanks a lot!

@pirj pirj requested review from bquorning and Darhazer June 29, 2020 07:55
Copy link
Member

@Darhazer Darhazer left a comment

Choose a reason for hiding this comment

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

You need to add s Changelog entry though

@harrylewis
Copy link
Contributor Author

@Darhazer I added a CHANGELOG entry. I used the "Fix" verb because the bug label was applied to the original issue.

Also, regarding the checklist item "squashed related commits together", I'm not sure if it means to squash all commits in the branch into one, or if it is good enough to make sure the PR is merged into master with a squash commit. Happy to do either!

@harrylewis harrylewis requested review from Darhazer and pirj June 29, 2020 16:47
@harrylewis harrylewis force-pushed the bugfix/add-new-factory-bot-method-to-allow-list branch from 241f3b5 to e2a8382 Compare June 29, 2020 16:49
@harrylewis
Copy link
Contributor Author

I didn't realize at first what was required to fill out a CHANGELOG entry. I really appreciate the accompanying tests though. They helped me understand was was required!

@pirj pirj merged commit 87229cc into rubocop:master Jun 29, 2020
@harrylewis harrylewis deleted the bugfix/add-new-factory-bot-method-to-allow-list branch June 29, 2020 19:23
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.

Add #traits_for_enum to RuboCop::RSpec::FactoryBot.reserved_methods
3 participants