New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

round before calculating exponent in number_to_human_converter #26628

Merged
merged 2 commits into from May 29, 2017

Conversation

Projects
None yet
8 participants
@mjhoy
Contributor

mjhoy commented Sep 26, 2016

Summary

Fixes #25664

Introduced in: 597a927

Other PRs:

I think the real problem is that NumberToHumanConverter does not round before calling calculate_exponent, which led to the confusing workaround that introduced #25664 in the first place.

Unfortunately the code to round in NumberToRoundedConverter (with options for precision and significant digits) is tied to formatting it as a string. The motivation for adding a new class RoundingHelper is to extract this part of the code so it can reused in NumberToHumanConverter.

I am fairly new to the Rails codebase, so I'm unsure of where to put the helper class. Should it go in its own file? Should it be a module?

There is also the messy fact that NumberToHumanConverter is rounding twice. I suppose the formatting code could be extracted as well.

@rails-bot

This comment has been minimized.

rails-bot commented Sep 26, 2016

Thanks for the pull request, and welcome! The Rails team is excited to review your changes, and you should hear from @eileencodes (or someone else) soon.

If any changes to this PR are deemed necessary, please add them as extra commits. This ensures that the reviewer can see what has changed since they last reviewed the code. Due to the way GitHub handles out-of-date commits, this should also make it reasonably obvious what issues have or haven't been addressed. Large or tricky changes may require several passes of review and changes.

This repository is being automatically checked for code quality issues using Code Climate. You can see results for this analysis in the PR status below. Newly introduced issues should be fixed before a Pull Request is considered ready to review.

Please see the contribution instructions for more information.

@bquorning

In my opinion, since RoundedHelper is being used by other classes than NumberToRoundedConverter, it should be in its own file (activesupport/lib/active_support/number_helper/rounding_helper.rb). Probably it should be autoloaded from activesupport/lib/active_support/number_helper.rb like the other number converter files are.

I’m thinking, perhaps this PR should be 2 commits: 1) extracting RoundedHelper, and 2) fixing the bug in NumberToHumanConverter.

In the Rails codebase, the convention is to indent all code following the private keyword with 2 extra spaces, and to have a whitespace inside { and } in hashes – which is why the build fails ;-)

activesupport/test/number_helper_test.rb Outdated
@@ -344,6 +344,9 @@ def test_number_to_human_with_custom_units
#Spaces are stripped from the resulting string
assert_equal "4", number_helper.number_to_human(4, units: { unit: "", ten: "tens " })
assert_equal "4.5 tens", number_helper.number_to_human(45, units: { unit: "", ten: " tens " })
#Uses only the provided units and does not try to use larger ones
assert_equal "1000 kilometers", number_helper.number_to_human(1_000_000, units: {unit: "meter", thousand: "kilometers"})

This comment has been minimized.

@bquorning

bquorning Sep 26, 2016

Contributor

May I suggest you also add these examples (in the “gangster” group, right after line 341):

assert_equal "1000 hundred bucks", number_helper.number_to_human(100_000, :units => gangster)
assert_equal "1 thousand quids", number_helper.number_to_human(999_999, :units => gangster)
assert_equal "1 thousand quids", number_helper.number_to_human(1_000_000, :units => gangster)

The first 2 fail on master, exposing another bug introduced by #20872. All 3 pass using your code.

activesupport/lib/active_support/number_helper/number_to_rounded_converter.rb Outdated
if options.delete(:significant) && precision > 0

This comment has been minimized.

@bquorning

bquorning Sep 26, 2016

Contributor

I really like that you’ve removed these delete calls ❤️

activesupport/lib/active_support/number_helper/number_to_rounded_converter.rb Outdated
def initialize(number, options)
@number = number
@options = options
if precision
case number

This comment has been minimized.

@bquorning

bquorning Sep 26, 2016

Contributor

Good call on moving this part into a constructor. It also means that line 7 (@number = number) can be moved to an else clause to the if precision call.

activesupport/lib/active_support/number_helper/number_to_rounded_converter.rb Outdated
end
rounded_number
end

This comment has been minimized.

@bquorning

bquorning Sep 26, 2016

Contributor

I’m wondering if this method reads better without the rounded_number local variables. What do you think?

def round
  if precision
    if options[:significant] && precision > 0
      round_significant
    else
      rounded_number = number.round(precision)
      rounded_number = rounded_number.to_i if precision == 0 && rounded_number.finite?
      rounded_number = rounded_number.abs if rounded_number.zero? # prevent showing negative zeros
      rounded_number
    end
  else
    number
  end
end

This comment has been minimized.

@mjhoy

mjhoy Sep 26, 2016

Contributor

The nested ifs confuse me a little there, but perhaps just because I don't usually write my code like that.

Another idea I had was

def round
  return number unless precision

  if options[:significant] && precision > 0
    round_significant
  else
    rounded_number = number.round(precision)
    rounded_number = rounded_number.to_i if precision == 0 && rounded_number.finite?
    rounded_number = rounded_number.abs if rounded_number.zero? # prevent showing negative zeros
    rounded_number
  end
end

This comment has been minimized.

@bquorning

bquorning Sep 26, 2016

Contributor

The nested ifs are indeed confusing. Extracting the else part to a method may make the rest acceptable.

I'm also thinking: we have a special case for 'if precisionin both this method and the initializer. Should we have an e.g.NoPrecisionRounder.new(number)` to return in this case?

This comment has been minimized.

@mjhoy

mjhoy Sep 26, 2016

Contributor

@bquorning I'm wondering if the method round (or maybe rounded) should just be an accessor to an instance variable set up in the initializer, where the rounding happens, so there is just one big if statement there.

@mjhoy mjhoy force-pushed the mjhoy:fix-number-to-human-25742 branch 2 times, most recently Sep 26, 2016

@mjhoy

This comment has been minimized.

Contributor

mjhoy commented Sep 26, 2016

@bquorning OK, see what you think, I think I got to all your comments, and I tried to get rid of the duplicated branching on precision as well.

activesupport/lib/active_support/number_helper/rounding_helper.rb Outdated
else
@number = number
end
end

This comment has been minimized.

@bquorning

bquorning Sep 27, 2016

Contributor

This has become quite a complex initializer. I actually think it would be good to keep the rounding functionality separate from the initializer (I know I said the opposite in the previous comment – sorry). It’s also confusing that after the initialization, @number is a BigDecimal if precision is on, but it may be a string or an integer or whatever if precision if off.

activesupport/lib/active_support/number_helper/rounding_helper.rb Outdated
end
def digit_count(number)
number.zero? ? 1 : (Math.log10(absolute_number(number)) + 1).floor

This comment has been minimized.

@bquorning

bquorning Sep 27, 2016

Contributor

I think a guard clause return 1 if number.zero? would be ok here.

activesupport/lib/active_support/number_helper/rounding_helper.rb Outdated
end
def round_significant
if zero?

This comment has been minimized.

@bquorning

bquorning Sep 27, 2016

Contributor

return 0 if zero??

activesupport/lib/active_support/number_helper/rounding_helper.rb Outdated
rounded_number = number.round(precision)
rounded_number = rounded_number.to_i if precision == 0 && rounded_number.finite?
rounded_number = rounded_number.abs if rounded_number.zero? # prevent showing negative zeros
rounded_number

This comment has been minimized.

@bquorning

bquorning Sep 27, 2016

Contributor

Should we extract lines 32–35 to a method?

@mjhoy

This comment has been minimized.

Contributor

mjhoy commented Sep 27, 2016

thanks @bquorning -- quick question about making changes, rails bot seems to suggest adding more commits as the review happens, and then squashing at the end?

@eileencodes

This comment has been minimized.

Member

eileencodes commented Sep 27, 2016

@mjhoy it's up to you. If you want to fixup/squash as you commit that's fine too. We prefer a squash at the end because it doesn't make sense to merge 3 commits for a single change. GitHub allows us to squash before merge too so it's really not a big deal 😄

@mjhoy

This comment has been minimized.

Contributor

mjhoy commented Sep 27, 2016

@eileencodes I prefer squashing as I go, but the bot seemed to suggest that reviewers wouldn't be able to follow as well if I did. Did the new review system fix that?

@eileencodes

This comment has been minimized.

Member

eileencodes commented Sep 27, 2016

The new review system does make it easier to see what's changed. For larger PR's (big features or yak shaves) it makes more sense to add individual commits.

But I'm assigned this PR and I almost never look at individual commits and usually look at the PR as a whole so I don't mind if you add more commits.

@mjhoy mjhoy force-pushed the mjhoy:fix-number-to-human-25742 branch Sep 27, 2016

@mjhoy

This comment has been minimized.

Contributor

mjhoy commented Sep 27, 2016

@bquorning Thanks for the input, I definitely see what you mean about the initializer. I restructured the class a bit, removing the @number instance variable altogether, and broke out some (hopefully more descriptive) methods. I also removed the zero? method as it had a needless respond_to check (the number at that point is guaranteed to be a BigDecimal). I took out the one line calculate_rounded_number(multiplier) method which I think had a confusing name, and moved the code to the round_significant method, as it only applies to calculating the rounded number with respect to significant digits.

@bquorning

This comment has been minimized.

Contributor

bquorning commented Sep 27, 2016

👍 I like this solution.

activesupport/lib/active_support/number_helper/rounding_helper.rb Outdated
def round_without_significant(number)
number = number.round(precision)
number = number.to_i if precision == 0 && number.finite?
number = number.abs if number.zero?

This comment has been minimized.

@bquorning

bquorning Sep 28, 2016

Contributor

Perhaps number = 0 if number.zero? is easier to understand?

Either way, I think this line is obscure enough that you should keep the prevent showing negative zeros comment.

This comment has been minimized.

@mjhoy

mjhoy Sep 28, 2016

Contributor

Agreed, I removed that unintentionally, whoops!

@mjhoy mjhoy force-pushed the mjhoy:fix-number-to-human-25742 branch Sep 28, 2016

def convert_to_decimal(number)
case number
when Float, String
number = BigDecimal(number.to_s)

This comment has been minimized.

@esparta

esparta Oct 11, 2016

Contributor

Since ruby is returning the last executed code, I think there's no need of the assignment. Does it?

case number
when Float, String
  BigDecimal(number.to_s)
when Rational
  BigDecimal(number, digit_count(number.to_i) + precision)
else
  number.to_d
end

This comment has been minimized.

@bquorning
@mjhoy

This comment has been minimized.

Contributor

mjhoy commented Oct 22, 2016

@esparta Thanks, I agree. However I would like to know whether this solution is even likely to be merged before working on it more. ( @eileencodes ?)

@eileencodes

This comment has been minimized.

Member

eileencodes commented Nov 1, 2016

Out of the 3 PR's fixing the mentioned issue this one seems to best address concerns of "what are the edge cases we're not thinking of beyond the one reported issue".

I have a few style changes I'd like to see but before I ask you to do more work @mjhoy I want to see what @matthewd thinks about the implementation since he disagreed with the original. The original issue is also a blocker for 5.0.1. Let me know what you think @matthewd.

@bquorning

This comment has been minimized.

Contributor

bquorning commented Feb 27, 2017

I’d love to see this PR moving forward again.

@mjhoy Would you please rebase on master? There’s just one easy-to-resolve conflict caused by fe1f4b2.

@matthewd Perhaps you overlooked the request from Eileen earlier. Would you have time to take a look at this PR, please?

@mjhoy mjhoy force-pushed the mjhoy:fix-number-to-human-25742 branch Mar 16, 2017

@mjhoy

This comment has been minimized.

Contributor

mjhoy commented Mar 16, 2017

@bquorning Sure.

@mjhoy mjhoy force-pushed the mjhoy:fix-number-to-human-25742 branch to 202aadd Mar 16, 2017

@daniel-rikowski

This comment has been minimized.

daniel-rikowski commented Mar 28, 2017

Is there a chance to have this in Rails 5.1? 🤞 The failed tests don't seem to be related to the code changes...

@bquorning

This comment has been minimized.

Contributor

bquorning commented Apr 18, 2017

Perhaps you overlooked the request from Eileen earlier. Would you have time to take a look at this PR, please?

Ping @matthewd

@matthewd

TBH I'd prefer it if we weren't introducing another object allocation, but this looks great!

@eileencodes eileencodes merged commit edc90c8 into rails:master May 29, 2017

2 checks passed

codeclimate no new or fixed issues
Details
continuous-integration/travis-ci/pr The Travis CI build passed
Details

eileencodes added a commit that referenced this pull request May 29, 2017

Merge pull request #26628 from mjhoy/fix-number-to-human-25742
round before calculating exponent in number_to_human_converter
@eileencodes

This comment has been minimized.

Member

eileencodes commented May 29, 2017

Backported to 5.1 in 55c3e1f

@bquorning

This comment has been minimized.

Contributor

bquorning commented May 29, 2017

Thank you @eileencodes . FYI the previous attempts at solving this issue #25742 and #26433 can be closed, too.

@mjhoy mjhoy deleted the mjhoy:fix-number-to-human-25742 branch Sep 6, 2017

y-yagi added a commit to y-yagi/rails that referenced this pull request Sep 19, 2017

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