#5136 no need of ternary operation - Ruby on Rails

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

July 17th, 2010 @ 02:23 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.

July 17th, 2010 @ 08:10 AM

Neeraj Singh

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.

July 17th, 2010 @ 05:38 PM

Stephan Wehner

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

July 17th, 2010 @ 06:08 PM

Neeraj Singh

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.

July 17th, 2010 @ 09:05 PM

Neeraj Singh

Attached is patch. All the tests are passing.

5136_fix_for_edge.diff 1.09 KB

July 17th, 2010 @ 09:11 PM

Neeraj Singh

Assigned user changed from Neeraj Singh to José Valim

July 17th, 2010 @ 09:11 PM

Neeraj Singh

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

July 17th, 2010 @ 09:16 PM

Repository

State changed from open to resolved

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

[#5136 no need of ternary operation state:resolved]

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

July 18th, 2010 @ 10:36 AM

no need of ternary operation

Activity