Rails Ruby on Rails → 3.x

Comments and changes to this ticket

• 

  Rohit Arondekar July 17th, 2010 @ 02:23 AM

  +1 hehe how did that get in there? :P

• You flagged this item as spam.
  

  Stephan Wehner July 17th, 2010 @ 08:10 AM

  Short note: the variable leading_zeros will be false if option[:leading_zeros] == true

  Options are only used with {... :leading_zeros => false ... } so it's not a problem right now. Doesn't look proper though.

• 

  Neeraj Singh July 17th, 2010 @ 05:38 PM

  • Tag changed from “rails 3” to “rails 3, datehelper”
  • Assigned user changed from “José Valim” to “Neeraj Singh”

  I took a deeper into datehelper and it is totally broken for option 'leading_zeros'. check this out

  {:leading_zeros=>false, :object=>#<Developer id: nil, name: nil, dob: nil, created_at: nil, updated_at: nil>}
  type is minute
  leading_zeros is true
  type is hour
  leading_zeros is true
  type is day
  leading_zeros is false
  type is year
  leading_zeros is false
  
  
  
  {:leading_zeros=>true, :object=>#<Developer id: nil, name: nil, dob: nil, created_at: nil, updated_at: nil>}
  type is minute
  leading_zeros is true
  type is hour
  leading_zeros is true
  type is day
  leading_zeros is false
  type is year
  leading_zeros is false
  

  As you can see in one case I am passing leading_zeros 'false' and in another case 'true'. And the values being passed to building blocks is not the value passed by user.

  I will create a code and test patch. Assigning it to myself until I have the fix ready.

• You flagged this item as spam.
  

  Stephan Wehner July 17th, 2010 @ 06:08 PM

  Oh ok Neeraj, I guess I thought :leading_zeros was an internal option.

• 

  Neeraj Singh July 17th, 2010 @ 09:05 PM

  This is what I found.

  :leading_zeros is an option that is not mentioned in the doc API so it is an internal option. This option is set only in following two methods.

  def select_day
    ...
    build_options_and_select(:day, day, :start => 1, 
                             :end => 31, :leading_zeros => false)
    ...
  end
  
  def select_year
    ...
    options[:leading_zeros] = false
    build_options_and_select(:year, val, options)
    ...
  end
  

  The method

  #leading_zeros = options.delete(:leading_zeros).nil? ? true : false
  

  worked because no one ever sets :leading_zeros => true. However if someone changes :leading_zeros => true then it will be treated as :leading_zeros => false because true.nil? is false.

  I am fixing it by replacing that line with the following two lines. This fix does not change existing behavior.
  However if someone passes :leading_zeros => true then that would be obeyed.

    options.reverse_merge!({:leading_zeros => true})
    leading_zeros = options.delete(:leading_zeros)
  

  Moving on to something unrelated.

  As of current code in the drop down for day you wil always see '1' not '01'. :leading_zeros => false is hardcoded so passing an option will have no impact.

  In minutes you will see '01' by default. However if you pass :leading_zeros => true it will have an impact.

  I am going to create another ticket which would list :leading_zeros as an option to end user and all attributes: year, month, day, hour, minutes and secods would obey :leading_zeros option.

• 

  Neeraj Singh July 17th, 2010 @ 09:11 PM

  Attached is patch. All the tests are passing.

  • 5136_fix_for_edge.diff 1.1 KB
• 

  Neeraj Singh July 17th, 2010 @ 09:11 PM

  • Assigned user changed from “Neeraj Singh” to “José Valim”
• 

  Neeraj Singh July 17th, 2010 @ 09:16 PM

  I created another ticket #5140 to deal with the inconsistency regarding :leading_zeros option. But this ticket and that patch attached is still valid and can be applied.

• 

  Repository July 18th, 2010 @ 10:36 AM

  • State changed from “open” to “resolved”

  (from [1f499e6d4cb1055de952957e3c9bd770e0219cc1]) fixing the ternary operation where the logic is very confusing.

  [#5136 state:resolved]

  Signed-off-by: José Valim jose.valim@gmail.com
  http://github.com/rails/rails/commit/1f499e6d4cb1055de952957e3c9bd7...