add :new_if_blank option to fields_for for nested associations

The patch looks good! Although I only had time to look at the patch atm, so disregard any notes if I didn't understand it well enough ;-)

Instead of using:

(args[:new_if_blank].to_i rescue 1).times do
   nested_association << klass.new
end

I'd prefer:

(args[:new_if_blank].is_a?(Numeric) ? args[:new_if_blank] : 1).times do
   nested_association << klass.new
end

As there is always the chance of masking real exceptions when using a inline rescue. Plus the fact that args[:new_if_blank] can be, for instance, 'true' which is not exceptional.

Personally I'd refactor that area somewhat like the following. But I will have a more thorough pass at this once I apply the patch anyways, so it doesn't really matter for now.

def build_nested_association(association_name, times)
  klass = association_name.to_s.classify.constantize
  if @object.send(association_name).is_a?(Array)
    Array.new(times.is_a?(Numeric) ? times : 1) { klass.new }
  else
    klass.new
  end
end

...
if association.blank? && args.last.is_a?(Hash) && (times = args.last[:new_if_blank])
  association = build_nested_association(association_name, times)
end

Also, as I understand it there will still be a NoMethodError raised if there's no single association build right?

I think that in this case we should not yield a new form builder at all, as is the case with an empty collection association. This was already being discussed on #2172 Nested model forms error on loading data from belongs_to associations that ev.... Thoughts?

Oh btw, normally I'd refrain from theorizing. But I wonder if it might be a good idea to have ":new => 3" as an option for a collection association as well, which would always add 3 new objects.

I think the refactorings are spot on. The fact that :new_is_blank can be true is not really exceptional in this case. Also build_nested_association is now a little easier on the eyes.

Here's an updated patch based on Eloy's suggestions for refactoring.

I'm not sure I like the idea of the option ":new => 3" for collection associations. I'd have to play around with it to get a real feel for it.

Guys, wouldn't be better to use :build_if_blank? We are building an object in such cases...

I was originally calling the param create_blank but I can't for the life of me remember why we renamed it. Eloy do you recall?

@Jose

I believe the reason we went with "new" over "build" (or "create") is because Eloy wants to stay ORM agnostic. "build" is an AR convention so we went with "new". If you look at the patch it doesn't actually call build it calls new.

(hopefully Eloy will correct me if I got it wrong)

(hopefully Eloy will correct me if I got it wrong)

You are spot on good sir :)

agreed and understood! :)

Association array determination is a problem I looked at when putting together my patch #2648 Accept arrays for nested attributes fields_for. As far as I can tell the is_a? will not be intercepted by the collection proxy so it will instantiate the association. I found it hard to find a neat way around this while still keeping the view code ORM agnostic. I did consider checking for "#{association_name}_ids=" method or adding is_a? to the proxy collection but neither seem like great ideas.

This option is hard to understand and harder to discover. I'm worried it's making the API muddier not clearer.

When would you want :build_if_blank => false?

In fact, iirc, I only needed that once. And it was quite the edge case. So am I correct in understanding that you mean; ”default to build if there's no associated record“ ?

If so, I think you're right. I would still like to be able to disable it though. And for a collection association you might want build more than 1 record. Although in retrospect, we could default this to just one as well. At least for now.

私の中で、総合評価のとっても低いアバアバクロホリスタークロ銀座店。アバクロは大好きなんですけどね。一昨日の東京駅付近での打ち合わせの後、散歩がてら久々に行ってきました。そしたらビックリ！相変わらアバクロず、踊っているだけの店員さんとかもいましたが、