Cleaning up a few lines #8420

Closed
wants to merge 1 commit into
from

Projects

None yet

5 participants

@ershad

Refactored some unnecessary '? true : false' statements and fixed a few spacing issues

@jeremy
Ruby on Rails member

Thanks for the tweaks!

Looks like some of these are to ensure a boolean return value. Does that affect these API in any other way? Do tests pass? Are any of the changes to public API, where a developer may be relying on a boolean return value?

@ershad

True, there's such a case. All tests are passing and the following are my thoughts on changes in files:

  1. I think the change in 'actionpack/lib/action_dispatch/routing/mapper.rb' is okay since 'merge_shallow_scope' is a private method and it's not invoked anywhere.

  2. activesupport/bin/generate_tables : The changes expect fixes for spacing issues were in a commented line :)

  3. activesupport/lib/active_support/multibyte/unicode.rb : It seems unsafe, I've reverted the changes back.

@jherdman jherdman and 1 other commented on an outdated diff Dec 5, 2012
activesupport/bin/generate_tables
@@ -61,10 +61,10 @@ module ActiveSupport
codepoint.combining_class = Integer($4)
#codepoint.bidi_class = $5
codepoint.decomp_type = $7
- codepoint.decomp_mapping = ($8=='') ? nil : $8.split.collect { |element| element.hex }
- #codepoint.bidi_mirrored = ($13=='Y') ? true : false
- codepoint.uppercase_mapping = ($16=='') ? 0 : $16.hex
- codepoint.lowercase_mapping = ($17=='') ? 0 : $17.hex
+ codepoint.decomp_mapping = ($8 == '') ? nil : $8.split.collect { |element| element.hex }
+ #codepoint.bidi_mirrored = ($13 == 'Y')
@jherdman
jherdman Dec 5, 2012

You may as well delete this one as it's commented out.

@ershad
ershad Dec 5, 2012

You mean remove the file from commit or delete that line? Thanks.

@pixeltrix
Ruby on Rails member

Can you leave the merge_shallow_scope one alone - it's there so that there is an explicit :shallow => true/false key in the scope rather than :shallow => nil. It's probably not going to break anything but I'd rather the hash remained consistent.

It is called, but it's masked by meta programming - see this line:
https://github.com/rails/rails/blob/master/actionpack/lib/action_dispatch/routing/mapper.rb#L657

@ershad

@pixeltrix I see. That file has been removed from the commit.

@pixeltrix
Ruby on Rails member

@ershad thanks

@carlosantoniodasilva
Ruby on Rails member

Thanks @ershad, but now I'd say it looks more like a cosmetic change, which we usually don't accept because it makes it harder to track the changes (same as removing white spaces for instance). If it's part of a bigger change related to that code, it's usually fine. Thanks again!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment