Rethink Active Form's structure #18
Conversation
| @@ -1,8 +1,7 @@ | |||
| require 'active_form/base' | |||
| require 'active_form/form' | |||
| require 'active_form/form_collection' | |||
| require 'active_form/collection_form' | |||
|
I loved this ❤️ @m-Peter wdyt? |
|
/cc @kirs |
|
@guilleiguaran I see that @kaspth has done a great job!!! |
|
"By also reflecting the hierarchy of a Rails params hash more explicitly in the code, we gain simpler code that is just delegation most of the time (see attributes= on the collection and model association classes)." Also liked this approach, we can use it to create dedicated classes that know how to populate the various types of association (:has_one, :has_many etc). This would help to refactor the AbstractForm#submit method. |
|
Thanks for the kind words, @guilleiguaran @m-Peter ❤️ @m-Peter actually |
|
Looks good, thank you for the PR! |
|
Yes and no. The refactorings were, but the last commit is where the real rethinking is. It was a mistake on my part to bundle these things together. Sorry. Kasper
|
| false | ||
| return false unless valid? | ||
|
|
||
| ActiveRecord::Base.transaction do |
There was a problem hiding this comment.
AFAIK .save doesn't need to be wrapped into transaction (it should be done automatically)
|
@kaspth please rebase this!!! |
Rename to model_class and make it private.
Inline declare_form and declare_form_collection methods. Avoid an extra Proc allocation.
Readability is damaged when variables with the same name mean different things.
FormCollection means that it's a collection of forms - which every form can do. CollectionForm means it's a form for collections - i.e. more than one association.
Define association getters using a common interface to avoid case statement.
Delete get_model outright in Form and CollectionForm classes.
Avoid using a callback. Rename `update_models` to `reset` for clearer intent.
|
@guilleiguaran done 😄 There's a problem though. When Ruby evaluates the class (and therefore calls class methods) and an So whenever a form is instantiated (like Meaning all forms share the same collection association model instances. Sorry, if it's not clear, I'm a little 😪 |
| if nested_params?(value) | ||
| assign_association_attributes(key, value) | ||
| else | ||
| send("#{key}=", value) |
There was a problem hiding this comment.
Probably could. I'll try it out.
|
Thanks for the review, @egilburg 😄 Actually I think it would be better to separate out the two parts I have here to two PRs. The refactoring and the rethinking (this one is the latter). I'll close this for now and open a new PR today. |
Hi @m-Peter!
Like I said I've been taking a look at Active Form and I'd love to help out 😄
All but the last commit are just refactorings trying to improve the readability. I was trying to understand the relationship between parts to better know what was going on.
The final commit tries to rethink what Active Form is and how it does things. It's definitely not done and all the tests don't pass.
The main realization came when I figured out that the
main_modelwas just another association. This means we can move a lot of logic to just that class and reason about it a lot better.By also reflecting the hierarchy of a Rails params hash more explicitly in the code, we gain simpler code that is just delegation most of the time (see
attributes=on the collection and model association classes).All of these are just suggestions and a different opinion 😄
What do you think?