Skip to content

Conversation

@composerinteralia
Copy link
Collaborator

Some people upgrade straight from factory_bot < 4.11 to factory_bot >=
5.0 and miss the deprecation cycle for static attributes. I have gotten
some feedback that the NoMethodError is confusing. Since we know that in
most cases people are seeing the NoMethodError when trying to define
static attributes, we offer them a "Did you mean?"-style suggestion for
how they might update to dynamic attributes.

I removed the extra test for setter methods. It was a remnant of
something I had removed in #1200. I see no reason to have special
treatment for setters at this point.

Closes #1272

@composerinteralia composerinteralia force-pushed the improve-static-attribute-error branch from e777ce4 to c48b1e9 Compare April 24, 2019 02:46
)
raise NoMethodError.new(<<~MSG)
undefined method '#{name}' in '#{@definition.name}' factory
Did you mean? '#{name} { #{args.first.inspect} }'
Copy link
Contributor

Choose a reason for hiding this comment

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

I see that args.first is used a few times in this method. Would it be worth extracting a variable to help describe why args.first is significant?

What do you think about:

def method_missing(name, *args, &block)
  attribute_value = Array(args).first  # coercing with `Array()` might not be necessary with splats

  if attribute_value.nil?
    # ...
  elsif attribute_value.respond_to?(:has_key?) && attribute_value.has_key?(:factory)
    # ...
  else
    # ...
  end
end

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

I like it. I adjusted the name a little and pulled out a private predicate method. What do you think:

def method_missing(name, *args, &block)
  association_options = args.first # AFAIK args will always be an array

  if association_options.nil?
    # ...
  elsif __valid_association_options?__(association_options)
    # ...
  else
    # ...
  end
end

private

def __valid_association_options?(options)
  options.respond_to?(:has_key?) && options.has_key?(:factory)
end

Some people upgrade straight from factory_bot < 4.11 to factory_bot >=
5.0 and miss the deprecation cycle for static attributes. I have gotten
some feedback that the NoMethodError is confusing. Since we know that in
most cases people are seeing the NoMethodError when trying to define
static attributes, we offer them a "Did you mean?"-style suggestion for
how they might update to dynamic attributes.

I removed the extra test for setter methods. It was a remnant of
something I had removed in #1200. I see no reason to have special
treatment for setters at this point.

Closes #1272
This code was a bit confusing before this change.

The only time we care about `args.first` is if we are creating an
association, so this PR gives `args.first` the name
`association_options` to reflect that.

The PR also pulls out a predicate method to give a name to the step of
validating that `args.first` are in indeed association options.
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.

Rails 5 - Undefined Method for all factories

3 participants