factory_bot icon indicating copy to clipboard operation
factory_bot copied to clipboard

Improve user feedback if defining static association with a block

Open grekko opened this issue 3 years ago • 6 comments

Problem this feature will solve

Given:

FactoryBot.define do
  user(factory: :author) { association(:author, role: role) }
end

DefinitionProxy#method_missing will declare the association and ignore any given block-argument.

Desired solution

DefinitionProxy#method_missing could warn users (this is just demo code :)):

def method_missing(name, *args, &block) # rubocop:disable Style/MissingRespondToMissing
  association_options = args.first

  if association_options.nil?
    __declare_attribute__(name, block)
  elsif __valid_association_options?(association_options)
    Kernel.puts 'warn: Dynamically provided attributes are ignored if `factory`-key is set.' if block
    association(name, association_options)
  else
    raise NoMethodError.new(<<~MSG)
      undefined method '#{name}' in '#{@definition.name}' factory
      Did you mean? '#{name} { #{association_options.inspect} }'
    MSG
  end
end

grekko avatar Jun 29 '21 13:06 grekko

Nice idea. What if we forward the block along:

elsif __valid_association_options?(association_options)
-  association(name, association_options)
+  association(name, association_options, &block)

So we can get the error already defined in the association method:

https://github.com/thoughtbot/factory_bot/blob/907139808d728dbe8da59856afe068b7c0699b2a/lib/factory_bot/definition_proxy.rb#L152-L156

That's technically a breaking change, so we may need to wait until factory_bot 7.

composerinteralia avatar Aug 10 '21 02:08 composerinteralia

So we can get the error already defined in the association method:

That look pretty clean to me 🤔 if you are interested, I could create a PR.

grekko avatar Aug 13 '21 16:08 grekko

Yeah, that would be great. Thanks!

composerinteralia avatar Aug 13 '21 17:08 composerinteralia

@composerinteralia I see grekko closed the PR. Was this feature implemented already? As I want to give it a try, is this still a good first issue? :)

sharvy avatar Jun 09 '23 12:06 sharvy

@sharvy I'm also tracking this issue. I think it was not implemented, see https://github.com/thoughtbot/factory_bot/pull/1518#issuecomment-1587525855.

tisba avatar Jun 13 '23 07:06 tisba

Hello 👋🏽

I see grekko closed the PR. Was this feature implemented already?

Yeah, I think I've prematurely cleaned up 🧹 the forked repository.

@sharvy I restored the fork and created a new PR which contains the proposed changes. see https://github.com/thoughtbot/factory_bot/pull/1579

grekko avatar Jun 13 '23 08:06 grekko