This project is archived and is in readonly mode.
no need of ternary operation
Reported by Neeraj Singh | July 17th, 2010 @ 12:46 AM | in 3.x
This is not a bug but code optimization.
http://github.com/neerajdotname/rails/commit/fd5011941bc9943e012293...
Comments and changes to this ticket
-
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.
-
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
- 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...
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>
People watching this ticket
Attachments
Tags
Referenced by
- 5136 no need of ternary operation [#5136 state:resolved]