[1452] update traits_for_enum to accept _prefix and _suffix #1513

tjbarker · 2021-09-20T06:00:17Z

in response to Issue.

goal is to allow _prefix and _suffix for enum trait definitions to stay in line with ruby implementations
also will not define the standard enum (without prefix/suffix) will only define the ones with prefix/suffix

jasonkarns · 2021-09-20T21:17:49Z

I came just to report this issue but it already exists, yay!

Glad to see work addressing it. Though as a user, I wouldn't be excited the current implementation requires duplicating the enum configuration in both the model and the factory. I would have expected that the factory "inherits" the enum configuration from the model itself. Otherwise, the prefix/suffix config could drift from what is defined in the model.

tjbarker · 2021-09-20T22:26:59Z

Hey jasonkarns.

not wanting to replicate enum config in factories and models makes sense. This implementation was chosen because the flip side of explicitly setting these values would require hooking into active record which currently does not store or make available enough information to build this out.

my reasoning for thinking it wouldn’t be that bad to use this is:

the factory only needs to define the enum and prefix/suffix. If you are inheriting the enum list from the model then it will continue to inherit. Change in model enums are usually due to adding values which would automatically be added to the factory’s trait list, it’s not often that the enum attribute name or prefix/suffix change
this will only be needed for enums with a prefix/suffix. Most enums don’t have these so will continue to be automatically added as traits to factories

composerinteralia

Excellent work! A few thoughts, but I think we should be able to support this.

spec/acceptance/enum_traits_spec.rb

composerinteralia · 2021-09-24T13:00:40Z

lib/factory_bot/definition_proxy.rb

@@ -230,8 +274,11 @@ def trait(name, &block)
    #     those traits. When this argument is not provided, factory_bot will
    #     attempt to get the values by calling the pluralized `attribute_name`
    #     class method.
-    def traits_for_enum(attribute_name, values = nil)
-      @definition.register_enum(Enum.new(attribute_name, values))
+    def traits_for_enum(attribute_name, values = nil, _prefix: false, _suffix: false, **values_as_hash)


Supporting both hash and keyword arguments is making this a bit complicated. I don't think we need to match the Rails enum method exactly here, since the use cases are a bit different (for example, I think passing the values to traits_for_enum as keyword arguments would be fairly uncommon, and it's also not something we've tested).

I'd rather accept a single values argument (like a hash or array), and keep the prefix and suffix keyword arguments separate. That also allows us to get rid of the ugly underscores.

So that would look like:

def traits_for_enum(attribute_name, values = nil, prefix: false, suffix: false) @definition.register_enum(Enum.new(attribute_name, values, prefix: prefix, suffix: suffix)) end

All of our tests still pass with this change (assuming we update the _prefix and _suffix keyword arguments to remove the underscores).

the issue here is not necessarily that key names are needed, but more that a third argument is needed for setting options, but the traits_for_enum allows for values to be nil (in which case the second arg passed becomes options, not values).
if traits_for_enum is invoked with values being passed in, a third arg with options works fine, but if values is not passed in, the "options" now are the second arg, meaning values (was meant to default to nil) and options (was meant to be passed in) are both jeopardised.

i can think of 2 options here,
1: the code update I've made, which looks good from an api usage point of view, but means traits_for_enum becomes messy, as it has to try to determine on the fly what the intention of the second arg is.

2: if "options" are being used, values is a key name too (eg: traits_for_enum(:foo, values: [:a, :b], prefix: true, suffix: true), which means the consumer of the api has to change their code (only when setting options, which are a new concept so all current usage continues to work) but will need to migrate over when wanting to set prefix/suffix.

also, making the change you suggested above fails the test on line 60 (because of the confusion about what teh second arg is)

composerinteralia · 2021-09-24T13:01:10Z

spec/acceptance/enum_traits_spec.rb

@@ -113,6 +113,211 @@ def each(&block)
        expect(task.status).to eq(trait_name)
      end
    end
+
+    context 'when prefix is used' do
+      context 'when values are a hash' do


Do prefix and suffixes interact differently with a hash vs. list, or is there something specific we're trying to cover by testing against both? If not, I think we could choose one or the other to test with, since we've already got coverage of the various enumerable types.

With the current form of the code, no they don't behave differently, however with hash being passed in one of the tests (enum_for_trait_spec.rb:60) was failing as the keynames of the hash didn't match with the prefix, suffix options, so i chose to leave the 2 different tests in (for now at least) to ensure i pushed a solution that worked for all implementation types of the api

composerinteralia · 2021-09-24T13:03:30Z

lib/factory_bot/enum.rb

@@ -12,14 +23,16 @@ def build_traits(klass)
      end
    end

+    attr_reader :attribute_name


Are we using this anywhere?

This was being used in lib/factory_bot/definition.rb to help with not redefining traits in the automatically_register_defined_enums method, it is now used in the Definition#register_enum method as the key in the registered_enum hash

this is now also being used in the registered_enums hash as the key to look up to ensure enums are not registered twice

lib/factory_bot/definition.rb

aledustet

Nice addition! I think the change would be less "disruptive" if the new options become extra arguments there and we don't support the values as options signature but I'm excited to bring this in. Thank you!

aledustet · 2021-09-24T14:19:44Z

lib/factory_bot/definition_proxy.rb

+    #
+    # Equivalent to:
+    #   factory :task do
+    #     traits_for_enum :status, [:started, :finished], _suffix: 'status'


Might be worth pointing out we would need to update the docs here if we end up using the signature without the hash arguments.

lib/factory_bot/definition.rb

…trait_for_enum to not take key names for options

[1452] update traits_for_enum to accept _prefix and _suffix

2eca2a4

tjbarker mentioned this pull request Sep 20, 2021

enum traits do not use prefix #1452

Open

composerinteralia reviewed Sep 24, 2021

View reviewed changes

aledustet reviewed Sep 24, 2021

View reviewed changes

[1452] move registered_enums to name => enum hash for lookup, update …

15ff7d7

…trait_for_enum to not take key names for options

tjbarker requested a review from composerinteralia June 13, 2022 00:16

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[1452] update traits_for_enum to accept _prefix and _suffix #1513

[1452] update traits_for_enum to accept _prefix and _suffix #1513

tjbarker commented Sep 20, 2021 •

edited

jasonkarns commented Sep 20, 2021

tjbarker commented Sep 20, 2021

composerinteralia left a comment

composerinteralia Sep 24, 2021

tjbarker Jun 12, 2022 •

edited

composerinteralia Sep 24, 2021

tjbarker Jun 12, 2022

composerinteralia Sep 24, 2021

tjbarker Oct 2, 2021 •

edited

tjbarker Jun 12, 2022

aledustet left a comment

aledustet Sep 24, 2021

[1452] update traits_for_enum to accept _prefix and _suffix #1513

Are you sure you want to change the base?

[1452] update traits_for_enum to accept _prefix and _suffix #1513

Conversation

tjbarker commented Sep 20, 2021 • edited

jasonkarns commented Sep 20, 2021

tjbarker commented Sep 20, 2021

composerinteralia left a comment

Choose a reason for hiding this comment

composerinteralia Sep 24, 2021

Choose a reason for hiding this comment

tjbarker Jun 12, 2022 • edited

Choose a reason for hiding this comment

composerinteralia Sep 24, 2021

Choose a reason for hiding this comment

tjbarker Jun 12, 2022

Choose a reason for hiding this comment

composerinteralia Sep 24, 2021

Choose a reason for hiding this comment

tjbarker Oct 2, 2021 • edited

Choose a reason for hiding this comment

tjbarker Jun 12, 2022

Choose a reason for hiding this comment

aledustet left a comment

Choose a reason for hiding this comment

aledustet Sep 24, 2021

Choose a reason for hiding this comment

tjbarker commented Sep 20, 2021 •

edited

tjbarker Jun 12, 2022 •

edited

tjbarker Oct 2, 2021 •

edited