Added documentation to the FormBuilder class #10122

Merged
merged 1 commit into from Apr 8, 2013

Conversation

Projects
None yet
4 participants
Contributor

wangjohn commented Apr 7, 2013

This should help clarify issue #10115. The documentation attempts to explain what the FormBuilder class does.

I've also changed the field_helpers attribute to an explicit list of methods. The reason I did this was because the old way of defining the field_helpers called FormHelper.instance_methods and subtracted away some methods. It is hard to figure out what methods these are without running the code, so I think that this change so make things more readable and understandable. Thoughts?

@wangjohn wangjohn Added documentation to the FormBuilder class, should help
clarify issue #10115. Also made the field_helpers an explicit list of
methods.
fb24f41
Contributor

egilburg commented Apr 7, 2013

+1

@egilburg egilburg commented on the diff Apr 7, 2013

actionpack/lib/action_view/helpers/form_helper.rb
@@ -1239,7 +1292,7 @@ def #{selector}(method, options = {}) # def text_field(method, options = {})
# Admin? : <%= permission_fields.check_box :admin %>
# <% end %>
#
- # <%= f.submit %>
+ # <%= person_form.submit %>
@egilburg

egilburg Apr 7, 2013

Contributor

You use person_form var name here, yet |f| in line 1195. Could they be made consistent?

@wangjohn

wangjohn Apr 7, 2013

Contributor

Ah, sorry this diff isn't very clear. The above is for the documentation for the fields_for method. Here's what it looked like before:

      #   <%= form_for @person do |person_form| %>
      #     First name: <%= person_form.text_field :first_name %>
      #     Last name : <%= person_form.text_field :last_name %>
      #
      #     <%= fields_for :permission, @person.permission do |permission_fields| %>
      #       Admin?  : <%= permission_fields.check_box :admin %>
      #     <% end %>
      #
      #     <%= f.submit %>
      #   <% end %>

I'm just changing the <%= f.submit %> to correct an error.

@egilburg egilburg commented on the diff Apr 7, 2013

actionpack/lib/action_view/helpers/form_helper.rb
class FormBuilder
include ModelNaming
# The methods which wrap a form helper call.
class_attribute :field_helpers
- self.field_helpers = FormHelper.instance_methods - [:form_for, :convert_to_model, :model_name_from_record_or_class]
@egilburg

egilburg Apr 7, 2013

Contributor

Is this just for clarity? It could be extra maintenance if they have to be changed in both places every time another field is added/removed.

@wangjohn

wangjohn Apr 7, 2013

Contributor

Yes, this is just for clarity. I was wondering about thoughts on this. Obviously, this method requires extra maintenance, but it might be worth the added clarity because (at least in my opinion), it makes it clearer what fields are being passed when the helper method wrappers are defined https://github.com/rails/rails/blob/master/actionpack/lib/action_view/helpers/form_helper.rb#L1206-1216.

Member

senny commented Apr 8, 2013

@rafaelfranca rafaelfranca added a commit that referenced this pull request Apr 8, 2013

@rafaelfranca rafaelfranca Merge pull request #10122 from wangjohn/adding_documentation_to_form_…
…builder

Added documentation to the FormBuilder class

Closes #10115
436d918

@rafaelfranca rafaelfranca merged commit 436d918 into rails:master Apr 8, 2013

wangjohn deleted the wangjohn:adding_documentation_to_form_builder branch Apr 8, 2013

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment