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.

Sign up for GitHub

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

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

Open
wants to merge 2 commits into
base: master
Choose a base branch
from
Open

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

Show file tree
Hide file tree
Changes from 1 commit
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
2eca2a4
[1452] update traits_for_enum to accept _prefix and _suffix
Sep 20, 2021
15ff7d7
[1452] move registered_enums to name => enum hash for lookup, update …
Jun 12, 2022
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
7 changes: 6 additions & 1 deletion lib/factory_bot/definition.rb
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -173,7 +173,12 @@ def expand_enum_traits(klass)
end

def automatically_register_defined_enums(klass)
klass.defined_enums.each_key { |name| register_enum(Enum.new(name)) }
klass.defined_enums.each_key do |name|
next if registered_enums.any? do |registered_enum|
tjbarker marked this conversation as resolved.
Show resolved Hide resolved
registered_enum.attribute_name.to_s == name
end
register_enum(Enum.new(name))
end
end

def automatically_register_defined_enums?(klass)
Expand Down
51 changes: 49 additions & 2 deletions lib/factory_bot/definition_proxy.rb
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -221,6 +221,50 @@ def trait(name, &block)
# end
# end
#
# Prefix and Suffixes can be used for trait definition.
# both can be set to true (prefix/suffix will be set to enum name), or to a custom string
#
# Example:
# factory :task do
# traits_for_enum :status, [:started, :finished], _prefix: true
# end
#
# Equivalent to:
# factory :task do
# traits_for_enum :status, [:started, :finished], _prefix: 'status'
# end
#
# Both Equivalent to:
# factory :task do
# trait :status_started do
# status { :started }
# end
#
# trait :status_finished do
# status { :finished }
# end
# end
#
# Example:
# factory :task do
# traits_for_enum :status, [:started, :finished], _suffix: true
# end
#
# Equivalent to:
# factory :task do
# traits_for_enum :status, [:started, :finished], _suffix: 'status'
Copy link
Contributor

Choose a reason for hiding this comment

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

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

# end
#
# Both Equivalent to:
# factory :task do
# trait :started_status do
# status { :started }
# end
#
# trait :finished_status do
# status { :finished }
# end
# end
#
# Arguments:
# attribute_name: +Symbol+ or +String+
Expand All @@ -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)
Copy link
Collaborator

Choose a reason for hiding this comment

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

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

Copy link
Author

@tjbarker tjbarker Jun 12, 2022 •
edited
Loading

Choose a reason for hiding this comment

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

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)

enum_prefix = _prefix || (values.delete(:_prefix) if values.respond_to?(:delete))
enum_suffix = _suffix || (values.delete(:_suffix) if values.respond_to?(:delete))
values ||= values_as_hash.present? ? values_as_hash : nil
@definition.register_enum(Enum.new(attribute_name, values, prefix: enum_prefix, suffix: enum_suffix))
end

def initialize_with(&block)
Expand Down
17 changes: 15 additions & 2 deletions lib/factory_bot/enum.rb
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,9 +1,20 @@
module FactoryBot
# @api private
class Enum
def initialize(attribute_name, values = nil)
def initialize(attribute_name, values = nil, prefix: false, suffix: false)
@attribute_name = attribute_name
@values = values
@prefix = if prefix == true
"#{@attribute_name}_"
elsif prefix
"#{prefix}_"
end

@suffix = if suffix == true
"_#{@attribute_name}"
elsif suffix
"_#{suffix}"
end
end

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

attr_reader :attribute_name
Copy link
Collaborator

Choose a reason for hiding this comment

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

Are we using this anywhere?

Copy link
Author

@tjbarker tjbarker Oct 2, 2021 •
edited
Loading

Choose a reason for hiding this comment

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

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

Copy link
Author

Choose a reason for hiding this comment

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

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


private

def enum_values(klass)
@values || klass.send(@attribute_name.to_s.pluralize)
end

def build_trait(trait_name, attribute_name, value)
Trait.new(trait_name) do
Trait.new("#{@prefix}#{trait_name}#{@suffix}") do
add_attribute(attribute_name) { value }
end
end
Expand Down
207 changes: 206 additions & 1 deletion spec/acceptance/enum_traits_spec.rb
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -57,7 +57,7 @@

FactoryBot.define do
factory :task do
traits_for_enum :status, statuses
traits_for_enum :status, **statuses
tjbarker marked this conversation as resolved.
Show resolved Hide resolved
end
end

Expand Down Expand Up @@ -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
Copy link
Collaborator

Choose a reason for hiding this comment

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

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.

Copy link
Author

Choose a reason for hiding this comment

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

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

context 'when prefix is a boolean' do
it "builds traits for each enumerated value, applies default prefix and does not build the auto enum trait" do
statuses = { queued: 0, started: 1, finished: 2 }
define_model("Task", status: :integer) do
enum status: statuses, _prefix: true
end

FactoryBot.define do
factory :task do
traits_for_enum :status, _prefix: true
end
end

statuses.each_key do |trait_name|
task = FactoryBot.build(:task, "status_#{trait_name}")

expect{ FactoryBot.build(:task, trait_name) }.to raise_error KeyError, /Trait not registered/
expect(task.status).to eq(trait_name.to_s)
end

Task.reset_column_information
end
end

context 'when prefix is a string' do
it "builds traits for each enumerated value, applies custom prefix and does not build the auto enum trait" do
statuses = { queued: 0, started: 1, finished: 2 }
define_model("Task", status: :integer) do
enum status: statuses, _prefix: 'foobar'
end

FactoryBot.define do
factory :task do
traits_for_enum :status, _prefix: 'foobar'
end
end

statuses.each_key do |trait_name|
task = FactoryBot.build(:task, "foobar_#{trait_name}")

expect{ FactoryBot.build(:task, trait_name) }.to raise_error KeyError, /Trait not registered/
expect(task.status).to eq(trait_name.to_s)
end

Task.reset_column_information
end
end
end

context 'when values are a list' do
context 'when prefix is a boolean' do
it "builds traits for each enumerated value, applies default prefix and does not build the auto enum trait" do
statuses = %w[queued started finished]

define_model("Task", status: :integer) do
enum status: statuses, _prefix: true
end

FactoryBot.define do
factory :task do
traits_for_enum :status, _prefix: true
end
end

statuses.each do |trait_name|
task = FactoryBot.build(:task, "status_#{trait_name}")

expect{ FactoryBot.build(:task, trait_name) }.to raise_error KeyError, /Trait not registered/
expect(task.status).to eq(trait_name)
end

Task.reset_column_information
end
end

context 'when prefix is a string' do
it "builds traits for each enumerated value, applies custom prefix and does not build the auto enum trait" do
statuses = %w[queued started finished]
define_model("Task", status: :integer) do
enum status: statuses, _prefix: 'foobar'
end

FactoryBot.define do
factory :task do
traits_for_enum :status, _prefix: 'foobar'
end
end

statuses.each do |trait_name|
task = FactoryBot.build(:task, "foobar_#{trait_name}")

expect{ FactoryBot.build(:task, trait_name) }.to raise_error KeyError, /Trait not registered/
expect(task.status).to eq(trait_name)
end

Task.reset_column_information
end
end
end
end

context 'when suffix is used' do
context 'when values are a hash' do
context 'when suffix is a boolean' do
it "builds traits for each enumerated value, applies default suffix and does not build the auto enum trait" do
statuses = { queued: 0, started: 1, finished: 2 }
define_model("Task", status: :integer) do
enum status: statuses, _suffix: true
end

FactoryBot.define do
factory :task do
traits_for_enum :status, _suffix: true
end
end

statuses.each_key do |trait_name|
task = FactoryBot.build(:task, "#{trait_name}_status")

expect{ FactoryBot.build(:task, trait_name) }.to raise_error KeyError, /Trait not registered/
expect(task.status).to eq(trait_name.to_s)
end

Task.reset_column_information
end
end

context 'when suffix is a string' do
it "builds traits for each enumerated value, applies custom suffix and does not build the auto enum trait" do
statuses = {queued: 0, started: 1, finished: 2}
define_model("Task", status: :integer) do
enum status: statuses, _suffix: 'foobar'
end

FactoryBot.define do
factory :task do
traits_for_enum :status, _suffix: 'foobar'
end
end

statuses.each do |trait_name, _trait_value|
task = FactoryBot.build(:task, "#{trait_name}_foobar")

expect{ FactoryBot.build(:task, trait_name) }.to raise_error KeyError, /Trait not registered/
expect(task.status).to eq(trait_name.to_s)
end

Task.reset_column_information
end
end
end

context 'when values are a list' do
context 'when suffix is a boolean' do
it "builds traits for each enumerated value, applies default suffix and does not build the auto enum trait" do
statuses = %w[queued started finished]
define_model("Task", status: :integer) do
enum status: statuses, _suffix: true
end

FactoryBot.define do
factory :task do
traits_for_enum :status, _suffix: true
end
end

statuses.each do |trait_name|
task = FactoryBot.build(:task, "#{trait_name}_status")

expect{ FactoryBot.build(:task, trait_name) }.to raise_error KeyError, /Trait not registered/
expect(task.status).to eq(trait_name)
end

Task.reset_column_information
end
end

context 'when suffix is a string' do
it "builds traits for each enumerated value, applies custom suffix and does not build the auto enum trait" do
statuses = %w[queued started finished]
define_model("Task", status: :integer) do
enum status: statuses, _suffix: 'foobar'
end

FactoryBot.define do
factory :task do
traits_for_enum :status, _suffix: 'foobar'
end
end

statuses.each do |trait_name|
task = FactoryBot.build(:task, "#{trait_name}_foobar")

expect(task.status).to eq(trait_name)
expect{ FactoryBot.build(:task, trait_name) }.to raise_error KeyError, /Trait not registered/
end

Task.reset_column_information
end
end
end
end
end

context "when automatically_define_enum_traits is false" do
Expand Down