tiny refactoring of date_helper #4739
Conversation
| @@ -854,7 +854,7 @@ def date_order | |||
| end | |||
|
|
|||
| def translated_date_order | |||
| date_order = I18n.translate(:'date.order', :locale => @options[:locale]) || [] | |||
| date_order = Array(I18n.translate(:'date.order', :locale => @options[:locale])) | |||
There was a problem hiding this comment.
Doesn't worth all the work that Kernel#Array does just to avoid || [], I like better the old version
There was a problem hiding this comment.
I think that is better to use the :default option of translate method
I18n.translate(:'date.order', :locale => @options[:locale], :default => [])There was a problem hiding this comment.
@rafaelfranca I've got these errors with :default option: https://gist.github.com/9a8634d407018d38d112
@spastorino reverted to old version.
There was a problem hiding this comment.
ah, I see. Thanks.
So, @spastorino, what do you prefer?
There was a problem hiding this comment.
agree with @rafaelfranca :)
There was a problem hiding this comment.
updated. Thanks guys for helping out with this!
There was a problem hiding this comment.
Thank you for improve the Rails code.
use :default option with translate method instead of condition simplify separator method
tiny refactoring of date_helper
| when :second | ||
| @options[:include_seconds] ? @options[:time_separator] : "" | ||
| when :minute, :second | ||
| @options[:"discard_#{type}"] ? "" : @options[:time_separator] |
There was a problem hiding this comment.
💣 These two can't be clubbed together. discard_minute, include_seconds.
There was a problem hiding this comment.
sorry, but what do you mean?
There was a problem hiding this comment.
Oh crap! You've changed the order of the ternary. I'm sorry. I read it wrongly as: you can't be returning @options[:time_separator] for the discard_seconds option.
There was a problem hiding this comment.
Hmmm. Thinking in this way the behavior was changed. Before this change the separator only is used if the option :include_seconds is true. Now the separator is not used if the option :discard_second is true. This mean that the separator is always used unless you had used the :discard_second option. This need to be fixed.
There was a problem hiding this comment.
But I think discard_second == not include_seconds or I'm missing something?https://github.com/nashby/rails/blob/master/actionpack/lib/action_view/helpers/date_helper.rb#L701
There was a problem hiding this comment.
Yeah, you are right. Sorry
use :default option with translate method instead of condition
simplify separator method