Skip to content

HTTPS clone URL

Subversion checkout URL

You can clone with
or
.
Download ZIP

Loading…

some improvements on ActionView::Helpers::FormHelper refactoring #4515

Closed
wants to merge 3 commits into from

5 participants

@amatsuda
Collaborator

Here are some tiny improvements on AV::Helpers' great refactoring done in #4488 :rocket:

  • removed needless module_eval

  • memoized field_type and select_type at class level

  • renamed AV::Helpers::Tags::Base#render to to_s

As for the render vs to_s method name, I did so because

  • what this method does is nothing but just a to_s
  • we probably can do something like <%= SomeTag.new obj, method %> in views, since ERB implicitly calls to_s

What do you think, @rafaelfranca?

@josevalim
Owner

Thanks for the pull. I am in general not a fan of overriding to_s. But let's see what other core members think about it. /cc @tenderlove @jonleighton @fxn

@jonleighton
Collaborator

I agree with @josevalim; render is domain specific terminology, whereas to_s is not. If these classes were public then maybe I would be persuaded otherwise, but I think we are agreed that they should remain private, hence there is no great advantage in making it easier for people to use in their erb files.

@rafaelfranca

:-1: to change render to to_s, for the same reasons that @jonleighton. But I liked the others changes.

@carlosantoniodasilva

I believe the most important changes were already applied by @rafaelfranca's pull request, so this issue could be closed.

@josevalim josevalim closed this
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
This page is out of date. Refresh to see the latest.
Showing with 54 additions and 40 deletions.
  1. +3 −1 actionpack/lib/action_view/helpers/active_model_helper.rb
  2. +3 −3 actionpack/lib/action_view/helpers/date_helper.rb
  3. +14 −14 actionpack/lib/action_view/helpers/form_helper.rb
  4. +4 −4 actionpack/lib/action_view/helpers/form_options_helper.rb
  5. +1 −1  actionpack/lib/action_view/helpers/tags/base.rb
  6. +1 −1  actionpack/lib/action_view/helpers/tags/check_box.rb
  7. +1 −1  actionpack/lib/action_view/helpers/tags/collection_select.rb
  8. +8 −2 actionpack/lib/action_view/helpers/tags/date_select.rb
  9. +1 −1  actionpack/lib/action_view/helpers/tags/file_field.rb
  10. +1 −1  actionpack/lib/action_view/helpers/tags/grouped_collection_select.rb
  11. +1 −1  actionpack/lib/action_view/helpers/tags/hidden_field.rb
  12. +1 −1  actionpack/lib/action_view/helpers/tags/label.rb
  13. +1 −1  actionpack/lib/action_view/helpers/tags/number_field.rb
  14. +1 −1  actionpack/lib/action_view/helpers/tags/password_field.rb
  15. +1 −1  actionpack/lib/action_view/helpers/tags/radio_button.rb
  16. +1 −1  actionpack/lib/action_view/helpers/tags/search_field.rb
  17. +1 −1  actionpack/lib/action_view/helpers/tags/select.rb
  18. +1 −1  actionpack/lib/action_view/helpers/tags/text_area.rb
  19. +8 −2 actionpack/lib/action_view/helpers/tags/text_field.rb
  20. +1 −1  actionpack/lib/action_view/helpers/tags/time_zone_select.rb
View
4 actionpack/lib/action_view/helpers/active_model_helper.rb
@@ -16,7 +16,9 @@ def object
end
end
- module_eval "def content_tag(*) error_wrapping(super) end", __FILE__, __LINE__
+ def content_tag(*)
+ error_wrapping(super)
+ end
def tag(type, options, *)
tag_generate_errors?(options) ? error_wrapping(super) : super
View
6 actionpack/lib/action_view/helpers/date_helper.rb
@@ -213,7 +213,7 @@ def time_ago_in_words(from_time, include_seconds = false)
# Note: If the day is not included as an option but the month is, the day will be set to the 1st to ensure that
# all month choices are valid.
def date_select(object_name, method, options = {}, html_options = {})
- Tags::DateSelect.new(object_name, method, self, options, html_options).render
+ Tags::DateSelect.new(object_name, method, self, options, html_options).to_s
end
# Returns a set of select tags (one for hour, minute and optionally second) pre-selected for accessing a
@@ -251,7 +251,7 @@ def date_select(object_name, method, options = {}, html_options = {})
# Note: If the day is not included as an option but the month is, the day will be set to the 1st to ensure that
# all month choices are valid.
def time_select(object_name, method, options = {}, html_options = {})
- Tags::TimeSelect.new(object_name, method, self, options, html_options).render
+ Tags::TimeSelect.new(object_name, method, self, options, html_options).to_s
end
# Returns a set of select tags (one for year, month, day, hour, and minute) pre-selected for accessing a
@@ -287,7 +287,7 @@ def time_select(object_name, method, options = {}, html_options = {})
#
# The selects are prepared for multi-parameter assignment to an Active Record object.
def datetime_select(object_name, method, options = {}, html_options = {})
- Tags::DatetimeSelect.new(object_name, method, self, options, html_options).render
+ Tags::DatetimeSelect.new(object_name, method, self, options, html_options).to_s
end
# Returns a set of html select-tags (one for year, month, day, hour, minute, and second) pre-selected with the
View
28 actionpack/lib/action_view/helpers/form_helper.rb
@@ -655,7 +655,7 @@ def fields_for(record_name, record_object = nil, options = {}, &block)
# 'Accept <a href="/terms">Terms</a>.'.html_safe
# end
def label(object_name, method, content_or_options = nil, options = nil, &block)
- Tags::Label.new(object_name, method, self, content_or_options, options).render(&block)
+ Tags::Label.new(object_name, method, self, content_or_options, options).to_s(&block)
end
# Returns an input tag of the "text" type tailored for accessing a specified attribute (identified by +method+) on an object
@@ -677,7 +677,7 @@ def label(object_name, method, content_or_options = nil, options = nil, &block)
# # => <input type="text" id="snippet_code" name="snippet[code]" size="20" value="#{@snippet.code}" class="code_input" />
#
def text_field(object_name, method, options = {})
- Tags::TextField.new(object_name, method, self, options).render
+ Tags::TextField.new(object_name, method, self, options).to_s
end
# Returns an input tag of the "password" type tailored for accessing a specified attribute (identified by +method+) on an object
@@ -699,7 +699,7 @@ def text_field(object_name, method, options = {})
# # => <input type="password" id="account_pin" name="account[pin]" size="20" class="form_input" />
#
def password_field(object_name, method, options = {})
- Tags::PasswordField.new(object_name, method, self, options).render
+ Tags::PasswordField.new(object_name, method, self, options).to_s
end
# Returns a hidden input tag tailored for accessing a specified attribute (identified by +method+) on an object
@@ -717,7 +717,7 @@ def password_field(object_name, method, options = {})
# hidden_field(:user, :token)
# # => <input type="hidden" id="user_token" name="user[token]" value="#{@user.token}" />
def hidden_field(object_name, method, options = {})
- Tags::HiddenField.new(object_name, method, self, options).render
+ Tags::HiddenField.new(object_name, method, self, options).to_s
end
# Returns a file upload input tag tailored for accessing a specified attribute (identified by +method+) on an object
@@ -738,7 +738,7 @@ def hidden_field(object_name, method, options = {})
# # => <input type="file" id="attachment_file" name="attachment[file]" class="file_input" />
#
def file_field(object_name, method, options = {})
- Tags::FileField.new(object_name, method, self, options).render
+ Tags::FileField.new(object_name, method, self, options).to_s
end
# Returns a textarea opening and closing tag set tailored for accessing a specified attribute (identified by +method+)
@@ -766,7 +766,7 @@ def file_field(object_name, method, options = {})
# # #{@entry.body}
# # </textarea>
def text_area(object_name, method, options = {})
- Tags::TextArea.new(object_name, method, self, options).render
+ Tags::TextArea.new(object_name, method, self, options).to_s
end
# Returns a checkbox tag tailored for accessing a specified attribute (identified by +method+) on an object
@@ -828,7 +828,7 @@ def text_area(object_name, method, options = {})
# # <input type="checkbox" class="eula_check" id="eula_accepted" name="eula[accepted]" value="yes" />
#
def check_box(object_name, method, options = {}, checked_value = "1", unchecked_value = "0")
- Tags::CheckBox.new(object_name, method, self, checked_value, unchecked_value, options).render
+ Tags::CheckBox.new(object_name, method, self, checked_value, unchecked_value, options).to_s
end
# Returns a radio button tag for accessing a specified attribute (identified by +method+) on an object
@@ -850,7 +850,7 @@ def check_box(object_name, method, options = {}, checked_value = "1", unchecked_
# # => <input type="radio" id="user_receive_newsletter_yes" name="user[receive_newsletter]" value="yes" />
# # <input type="radio" id="user_receive_newsletter_no" name="user[receive_newsletter]" value="no" checked="checked" />
def radio_button(object_name, method, tag_value, options = {})
- Tags::RadioButton.new(object_name, method, self, tag_value, options).render
+ Tags::RadioButton.new(object_name, method, self, tag_value, options).to_s
end
# Returns an input of type "search" for accessing a specified attribute (identified by +method+) on an object
@@ -876,7 +876,7 @@ def radio_button(object_name, method, tag_value, options = {})
# # => <input autosave="com.example.www" id="user_name" incremental="true" name="user[name]" onsearch="true" results="10" size="30" type="search" />
#
def search_field(object_name, method, options = {})
- Tags::SearchField.new(object_name, method, self, options).render
+ Tags::SearchField.new(object_name, method, self, options).to_s
end
# Returns a text_field of type "tel".
@@ -885,7 +885,7 @@ def search_field(object_name, method, options = {})
# # => <input id="user_phone" name="user[phone]" size="30" type="tel" />
#
def telephone_field(object_name, method, options = {})
- Tags::TelField.new(object_name, method, self, options).render
+ Tags::TelField.new(object_name, method, self, options).to_s
end
alias phone_field telephone_field
@@ -895,7 +895,7 @@ def telephone_field(object_name, method, options = {})
# # => <input id="user_homepage" size="30" name="user[homepage]" type="url" />
#
def url_field(object_name, method, options = {})
- Tags::UrlField.new(object_name, method, self, options).render
+ Tags::UrlField.new(object_name, method, self, options).to_s
end
# Returns a text_field of type "email".
@@ -904,7 +904,7 @@ def url_field(object_name, method, options = {})
# # => <input id="user_address" size="30" name="user[address]" type="email" />
#
def email_field(object_name, method, options = {})
- Tags::EmailField.new(object_name, method, self, options).render
+ Tags::EmailField.new(object_name, method, self, options).to_s
end
# Returns an input tag of type "number".
@@ -912,7 +912,7 @@ def email_field(object_name, method, options = {})
# ==== Options
# * Accepts same options as number_field_tag
def number_field(object_name, method, options = {})
- Tags::NumberField.new(object_name, method, self, options).render
+ Tags::NumberField.new(object_name, method, self, options).to_s
end
# Returns an input tag of type "range".
@@ -920,7 +920,7 @@ def number_field(object_name, method, options = {})
# ==== Options
# * Accepts same options as range_field_tag
def range_field(object_name, method, options = {})
- Tags::RangeField.new(object_name, method, self, options).render
+ Tags::RangeField.new(object_name, method, self, options).to_s
end
private
View
8 actionpack/lib/action_view/helpers/form_options_helper.rb
@@ -154,7 +154,7 @@ module FormOptionsHelper
# key in the query string, that works for ordinary forms.
#
def select(object, method, choices, options = {}, html_options = {})
- Tags::Select.new(object, method, self, choices, options, html_options).render
+ Tags::Select.new(object, method, self, choices, options, html_options).to_s
end
# Returns <tt><select></tt> and <tt><option></tt> tags for the collection of existing return values of
@@ -188,7 +188,7 @@ def select(object, method, choices, options = {}, html_options = {})
# <option value="3">M. Clark</option>
# </select>
def collection_select(object, method, collection, value_method, text_method, options = {}, html_options = {})
- Tags::CollectionSelect.new(object, method, self, collection, value_method, text_method, options, html_options).render
+ Tags::CollectionSelect.new(object, method, self, collection, value_method, text_method, options, html_options).to_s
end
# Returns <tt><select></tt>, <tt><optgroup></tt> and <tt><option></tt> tags for the collection of existing return values of
@@ -239,7 +239,7 @@ def collection_select(object, method, collection, value_method, text_method, opt
# </select>
#
def grouped_collection_select(object, method, collection, group_method, group_label_method, option_key_method, option_value_method, options = {}, html_options = {})
- Tags::GroupedCollectionSelect.new(object, method, self, collection, group_method, group_label_method, option_key_method, option_value_method, options, html_options).render
+ Tags::GroupedCollectionSelect.new(object, method, self, collection, group_method, group_label_method, option_key_method, option_value_method, options, html_options).to_s
end
# Return select and option tags for the given object and method, using
@@ -273,7 +273,7 @@ def grouped_collection_select(object, method, collection, group_method, group_la
#
# time_zone_select( "user", "time_zone", ActiveSupport::TimeZone.all.sort, :model => ActiveSupport::TimeZone)
def time_zone_select(object, method, priority_zones = nil, options = {}, html_options = {})
- Tags::TimeZoneSelect.new(object, method, self, priority_zones, options, html_options).render
+ Tags::TimeZoneSelect.new(object, method, self, priority_zones, options, html_options).to_s
end
# Accepts a container (hash, array, enumerable, your type) and returns a string of option tags. Given a container
View
2  actionpack/lib/action_view/helpers/tags/base.rb
@@ -19,7 +19,7 @@ def initialize(object_name, method_name, template_object, options = {})
@auto_index = retrieve_autoindex(Regexp.last_match.pre_match) if Regexp.last_match
end
- def render(&block)
+ def to_s(&block)
raise "Abstract Method called"
end
View
2  actionpack/lib/action_view/helpers/tags/check_box.rb
@@ -12,7 +12,7 @@ def initialize(object_name, method_name, template_object, checked_value, uncheck
super(object_name, method_name, template_object, options)
end
- def render
+ def to_s
options = @options.stringify_keys
options["type"] = "checkbox"
options["value"] = @checked_value
View
2  actionpack/lib/action_view/helpers/tags/collection_select.rb
@@ -11,7 +11,7 @@ def initialize(object_name, method_name, template_object, collection, value_meth
super(object_name, method_name, template_object, options)
end
- def render
+ def to_s
selected_value = @options.has_key?(:selected) ? @options[:selected] : value(@object)
select_content_tag(
options_from_collection_for_select(@collection, @value_method, @text_method, :selected => selected_value, :disabled => @options[:disabled]), @options, @html_options
View
10 actionpack/lib/action_view/helpers/tags/date_select.rb
@@ -8,14 +8,20 @@ def initialize(object_name, method_name, template_object, options, html_options)
super(object_name, method_name, template_object, options)
end
- def render
+ def to_s
error_wrapping(datetime_selector(@options, @html_options).send("select_#{select_type}").html_safe)
end
+ class << self
+ def select_type
+ @select_type ||= self.name.split("::").last.sub("Select", "").downcase
+ end
+ end
+
private
def select_type
- self.class.name.split("::").last.sub("Select", "").downcase
+ self.class.select_type
end
def datetime_selector(options, html_options)
View
2  actionpack/lib/action_view/helpers/tags/file_field.rb
@@ -2,7 +2,7 @@ module ActionView
module Helpers
module Tags
class FileField < TextField #:nodoc:
- def render
+ def to_s
@options.update(:size => nil)
super
end
View
2  actionpack/lib/action_view/helpers/tags/grouped_collection_select.rb
@@ -13,7 +13,7 @@ def initialize(object_name, method_name, template_object, collection, group_meth
super(object_name, method_name, template_object, options)
end
- def render
+ def to_s
select_content_tag(
option_groups_from_collection_for_select(@collection, @group_method, @group_label_method, @option_key_method, @option_value_method, value(@object)), @options, @html_options
)
View
2  actionpack/lib/action_view/helpers/tags/hidden_field.rb
@@ -2,7 +2,7 @@ module ActionView
module Helpers
module Tags
class HiddenField < TextField #:nodoc:
- def render
+ def to_s
@options.update(:size => nil)
super
end
View
2  actionpack/lib/action_view/helpers/tags/label.rb
@@ -16,7 +16,7 @@ def initialize(object_name, method_name, template_object, content_or_options = n
super(object_name, method_name, template_object, options)
end
- def render(&block)
+ def to_s(&block)
options = @options.stringify_keys
tag_value = options.delete("value")
name_and_id = options.dup
View
2  actionpack/lib/action_view/helpers/tags/number_field.rb
@@ -2,7 +2,7 @@ module ActionView
module Helpers
module Tags
class NumberField < TextField #:nodoc:
- def render
+ def to_s
options = @options.stringify_keys
options['size'] ||= nil
View
2  actionpack/lib/action_view/helpers/tags/password_field.rb
@@ -2,7 +2,7 @@ module ActionView
module Helpers
module Tags
class PasswordField < TextField #:nodoc:
- def render
+ def to_s
@options = {:value => nil}.merge!(@options)
super
end
View
2  actionpack/lib/action_view/helpers/tags/radio_button.rb
@@ -11,7 +11,7 @@ def initialize(object_name, method_name, template_object, tag_value, options)
super(object_name, method_name, template_object, options)
end
- def render
+ def to_s
options = @options.stringify_keys
options["type"] = "radio"
options["value"] = @tag_value
View
2  actionpack/lib/action_view/helpers/tags/search_field.rb
@@ -2,7 +2,7 @@ module ActionView
module Helpers
module Tags
class SearchField < TextField #:nodoc:
- def render
+ def to_s
options = @options.stringify_keys
if options["autosave"]
View
2  actionpack/lib/action_view/helpers/tags/select.rb
@@ -9,7 +9,7 @@ def initialize(object_name, method_name, template_object, choices, options, html
super(object_name, method_name, template_object, options)
end
- def render
+ def to_s
selected_value = @options.has_key?(:selected) ? @options[:selected] : value(@object)
# Grouped choices look like this:
View
2  actionpack/lib/action_view/helpers/tags/text_area.rb
@@ -4,7 +4,7 @@ module Tags
class TextArea < Base #:nodoc:
DEFAULT_TEXT_AREA_OPTIONS = { "cols" => 40, "rows" => 20 }
- def render
+ def to_s
options = DEFAULT_TEXT_AREA_OPTIONS.merge(@options.stringify_keys)
add_default_name_and_id(options)
View
10 actionpack/lib/action_view/helpers/tags/text_field.rb
@@ -2,7 +2,7 @@ module ActionView
module Helpers
module Tags
class TextField < Base #:nodoc:
- def render
+ def to_s
options = @options.stringify_keys
options["size"] = options["maxlength"] || DEFAULT_FIELD_OPTIONS["size"] unless options.key?("size")
options = DEFAULT_FIELD_OPTIONS.merge(options)
@@ -13,10 +13,16 @@ def render
tag("input", options)
end
+ class << self
+ def field_type
+ @field_type ||= self.name.split("::").last.sub("Field", "").downcase
+ end
+ end
+
private
def field_type
- @field_type ||= self.class.name.split("::").last.sub("Field", "").downcase
+ self.class.field_type
end
end
end
View
2  actionpack/lib/action_view/helpers/tags/time_zone_select.rb
@@ -9,7 +9,7 @@ def initialize(object_name, method_name, template_object, priority_zones, option
super(object_name, method_name, template_object, options)
end
- def render
+ def to_s
select_content_tag(
time_zone_options_for_select(value(@object) || @options[:default], @priority_zones, @options[:model] || ActiveSupport::TimeZone), @options, @html_options
)
Something went wrong with that request. Please try again.