This project is archived and is in readonly mode.

#5136 ✓resolved
Neeraj Singh

no need of ternary operation

Reported by Neeraj Singh | July 17th, 2010 @ 12:46 AM | in 3.x

Comments and changes to this ticket

  • Rohit Arondekar

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

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

  • Stephan Wehner

    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

    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.

  • Stephan Wehner

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

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

  • Neeraj Singh

    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

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

    Attached is patch. All the tests are passing.

  • Neeraj Singh

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

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

    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

    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...

Create your profile

Help contribute to this project by taking a few moments to create your personal profile. Create your profile »

<h2 style="font-size: 14px">Tickets have moved to Github</h2>

The new ticket tracker is available at <a href="https://github.com/rails/rails/issues">https://github.com/rails/rails/issues</a>

Attachments

Referenced by

Pages