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
Add functionality for enum traits #1380
Conversation
Co-authored-by: Lance Johnson <lancejjohnson@gmail.com>
* Case for building traits from any type of enum Refactored Enum#enum_values to get this test passing * Case for preferring user defined traits over automatically built traits
While I think it makes sense to default to enabling this feature, there may be cases where this is a breaking change and somebody wants to opt out of the feature. If somebody disables the automatic feature, they can still manually call `traits_for_enum`.
Before this commit the `Factory` class needed to know about both automatically registering enums and then expanding them. With this commit automatic registration is handled within a private method in the `Definition` so the `Factory` can remain ignorant of this step.
lib/factory_bot/factory.rb
Outdated
@@ -84,6 +84,7 @@ def compile | |||
unless @compiled | |||
parent.compile | |||
parent.defined_traits.each { |trait| define_trait(trait) } | |||
@definition.expand_enum_traits(build_class) | |||
@definition.compile |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
In an ideal world we could pass the build_class
to @definition.compile
and let the definition itself handle expanding the traits, but we call @definition.compile
in other places where a build_class
is not available.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
i agree with placing the triggering logic in @definition.compile
would be better, but i also agree that it may not worth it to refactor other parts largely only for this.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I pushed up 9c4d7c5, which solves this but adds an extra conditional to Definition#compile
. Do you think that is worth it?
Add documentation to DefinitionProxy#traits_for_enum, and include @api private comment to new Enum class.
- automatically_define_enum_traits = false - automatically_define_enum_traits = true When setting `FactoryBot.automatically_define_enum_traits = false`, we allow for 'turning off' automatically built traits from enum. Add test to confirm that attempting to build automatically while this is false raises an error.
lib/factory_bot/definition_proxy.rb
Outdated
# end | ||
# | ||
# Arguments: | ||
# * name_of_enum_type_attribute: +Symbol+ |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I don't think we use this name anywhere else. I think we should stick to the argument names we use in the method signature itself.
lib/factory_bot/definition_proxy.rb
Outdated
# traits_for_enum :status, statuses | ||
# end | ||
# | ||
# Arguments: |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I am finding it a little confusing to have multiple argument lists. Elsewhere I think we have consolidated them into one.
I am thinking:
# Arguments:
# attribute_name: +Symbol+ or +String+
# the name of the attribute these traits will set the value of
# values: +Array+, +Hash+, or other +Enumerable+
# An array of trait names, or a mapping of trait names to values for those traits. When this argument is not
# provided, factory_bot will attempt to get the values from the class of the object being built by calling the
# pluralized `attribute_name` class method.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It makes sense to consolidate, I was questioning whether or not having multiple argument lists would be valuable.
lib/factory_bot/definition_proxy.rb
Outdated
# * Where the Array has enum values | ||
# statuses = %w[queued started finished] | ||
# | ||
# * Where the CustomEnumClass has enum values |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I don't think we need this. This is teaching people how to build custom enumerables, which is beyond the scope of this documentation.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Fair point!
@fridaland I pushed up a commit to move some things around in the documentation based on my comments above. Let me know what you think! |
I still need to work on adding some documentation to the GETTING_STARTED guided, but I was waiting until #1381 gets reviewed. |
@composerinteralia updates look good. Thanks for clarifying the documentation! |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
other than the small things for tests (and again those are not very important and some even debatable) it looks good
lib/factory_bot/factory.rb
Outdated
@@ -84,6 +84,7 @@ def compile | |||
unless @compiled | |||
parent.compile | |||
parent.defined_traits.each { |trait| define_trait(trait) } | |||
@definition.expand_enum_traits(build_class) | |||
@definition.compile |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
i agree with placing the triggering logic in @definition.compile
would be better, but i also agree that it may not worth it to refactor other parts largely only for this.
expect(task.status).to eq(trait_name) | ||
end | ||
|
||
Task.reset_column_information |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
these may be better in an after hook so it's more readable (obviously not high prio)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This made me realize that not all of these tests need models, so I changed some in 1f70160 to use plain old Ruby objects instead. It no longer makes sense to call Task.reset_column_information
in an after hook since Task
won't have that method in every test.
context "when automatically_define_enum_traits is true" do | ||
it "builds traits automatically for model enum field" do | ||
define_model("Task", status: :integer) do | ||
enum status: { queued: 0, started: 1, finished: 2 } |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
these are somewhat also redundant, so they are used in every it
the same or just the keys as a string array, it may make it more readable to do a single let
for them for the whole describe
block (again obviously not high prio)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I am generally not a fan of let
for the reasons outlined in this post: https://thoughtbot.com/blog/lets-not.
end | ||
|
||
Task.reset_column_information | ||
end |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
to some extent the examples could be refactored to use a shared example, but i am not sure at what point it is just "clever" overengineering vs making it more readable. generally all the tests are doing
- define the "same" model with integer or string column, optionally adding an (AR) enum
- define the factory with or without explicit traits, obviously this is the main part, not strictly the subject of the tests but close (so the less code share available)
- assert the factory's traits building with the right attribute value
i would argue the first and last steps are very easy to share between the examples (but again, at some point it will be counter-productive), obviously low prio & more debatable
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
There might be some room to pull out some shared methods here, but lately I have been preferring repetition in my tests over clever shared behavior. I think https://thoughtbot.com/blog/the-case-for-wet-tests covers a lot of what I am thinking. I also talk a little bit about my preference for duplication in tests in https://www.bikeshed.fm/186.
This keeps Factory completely ignorant of enum traits. The downside is that it adds an extra conditional the Definition#compile, since trait definitions do not have access to the build class.
@eLod thanks so much for taking a look. I left some comments on your comments and pushed up two additional commits. |
This commit adds top-level documentation for the enum traits feature adding in #1380
This commit adds top-level documentation for the enum traits feature adding in #1380
This commit adds top-level documentation for the enum traits feature adding in #1380
This commit adds top-level documentation for the enum traits feature adding in #1380
Enum traits
This PR adds additional test coverage and finalizes the work started in #1301.
Given a Rails model with an enum attribute:
It is common for people to define traits for each possible value of the enum:
With this commit, those trait definitions are no longer necessary—they are defined automatically by factory_bot.
If automatically defining traits for enum attributes on every factory is not desired, it is possible to disable the feature by setting
FactoryBot.automatically_define_enum_traits = false
(see commit: Allow opting out of automatically defining traits).In that case, it is still possible to explicitly define traits for an enum attribute in a particular factory:
It is also possible to use this feature for other enumerable values, not specifically tied to ActiveRecord enum attributes:
The second argument here can be an enumerable object, including a Hash or Array.
Closes #1049