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

Address edge cases for number_to_human with units option. #9311

Closed
wants to merge 15 commits into
base: master
from

Conversation

Projects
None yet
10 participants
@hoffm
Contributor

hoffm commented Feb 16, 2013

ActiveSupport::NumberHelper#number_to_human returns the number unaltered when the units option is an empty hash or when its smallest key is larger than the number.

Examples:

number_to_human(123, :units => {}) # => 123
number_to_human(123, :units => {:thousand => 'k'}) # => 123

Fixes #9269.

Address edge cases for number_to_human with units option.
The number is returned unaltered in cases where:

- The value of the :units option is an empty hash.
- The smallest key of the units option is larger than the number.

Fixes #9269.
@sikachu

This comment has been minimized.

Show comment
Hide comment
@sikachu

sikachu Feb 17, 2013

Member

Actually, do you think this is an expected behavior, or do you think it should be fixed?

Member

sikachu commented Feb 17, 2013

Actually, do you think this is an expected behavior, or do you think it should be fixed?

@hoffm

This comment has been minimized.

Show comment
Hide comment
@hoffm

hoffm Feb 18, 2013

Contributor

My argument as two why it should be fixed (which I made briefly to @carlosantoniodasilva and @senny in #9269) is as follows.

First, it seems to me that, absent a reason to the contrary, number_to_human ought to return a value given any numeric input and valid options. The two cases this patch adresses violate this rule.

Second, this patch makes number_to_human more user-friendly in what I would imagine is a relatively common use case. Namely, cases in which one wants to display the number unaltered up to a certain value, and then abbreviate the number for bigger values. An example of such a case might be the display of follower and following numbers in a social app.

Before this fix, this use case requires something like:

if @follower_count < 1000
  @follower_count.to_s
else
  number_to_human(@follower_count, :precision => 2,
    :units => {:thousand => 'k', :million => 'M'})
end

With this patch, on the other hand, one can remove the if-clause altogether.

Contributor

hoffm commented Feb 18, 2013

My argument as two why it should be fixed (which I made briefly to @carlosantoniodasilva and @senny in #9269) is as follows.

First, it seems to me that, absent a reason to the contrary, number_to_human ought to return a value given any numeric input and valid options. The two cases this patch adresses violate this rule.

Second, this patch makes number_to_human more user-friendly in what I would imagine is a relatively common use case. Namely, cases in which one wants to display the number unaltered up to a certain value, and then abbreviate the number for bigger values. An example of such a case might be the display of follower and following numbers in a social app.

Before this fix, this use case requires something like:

if @follower_count < 1000
  @follower_count.to_s
else
  number_to_human(@follower_count, :precision => 2,
    :units => {:thousand => 'k', :million => 'M'})
end

With this patch, on the other hand, one can remove the if-clause altogether.

#Numbers smaller than smallest unit are returned unaltered
assert_equal '123', number_to_human(123, :units => {:thousand => 'k'})
assert_equal '123', number_to_human(123, :units => {})

This comment has been minimized.

@senny

senny Feb 19, 2013

Member

Is there a reason we stick everything in the same test-case? I think it's sad that you only get one failed assertion because it's all in the same test-case.

@rafaelfranca could you elaborate?

@senny

senny Feb 19, 2013

Member

Is there a reason we stick everything in the same test-case? I think it's sad that you only get one failed assertion because it's all in the same test-case.

@rafaelfranca could you elaborate?

This comment has been minimized.

@carlosantoniodasilva

carlosantoniodasilva Feb 19, 2013

Member

No problem in these cases I guess, but it's probably due to historical reasons.

@carlosantoniodasilva

carlosantoniodasilva Feb 19, 2013

Member

No problem in these cases I guess, but it's probably due to historical reasons.

This comment has been minimized.

@hoffm

hoffm Feb 19, 2013

Contributor

I also wondered why so many different assertions were grouped into one test. I wouldn't mind refactoring them into more granular tests as a separate pull request.

@hoffm

hoffm Feb 19, 2013

Contributor

I also wondered why so many different assertions were grouped into one test. I wouldn't mind refactoring them into more granular tests as a separate pull request.

This comment has been minimized.

@senny

senny Feb 20, 2013

Member

I'm not sure refactoring test-code is worth it. I would simply create a new test-case for your changes and choose an intention revealing name. We can then slowly create more test-cases as the code is changed.

@senny

senny Feb 20, 2013

Member

I'm not sure refactoring test-code is worth it. I would simply create a new test-case for your changes and choose an intention revealing name. We can then slowly create more test-cases as the code is changed.

wangjohn and others added some commits Feb 20, 2013

Renaming the check_empty_arguments method to something more descriptive.
The function is now called has_arguments? so that it's easier to tell
that it's just checking to see if the args are blank or not.
Merge pull request #9344 from senny/changelog_cleanup
unify AR changelog entries [ci skip]
Revert "Merge pull request #4803 from lucascaton/master"
This reverts commit bb842e8, reversing
changes made to 40c287c.
This was causing issues in one of our apps we just upgraded.
ActionController::RoutingError: No route matches [GET]
"/images/favicon.ico"
favicon_link_tag now returns '/images/favicon.ico' and in 3.2 returned
'/favicon.ico'
Browsers by default look for favicon.ico in the root directory

Conflicts:
	actionpack/CHANGELOG.md
	actionpack/lib/action_view/helpers/asset_tag_helper.rb
@guilleiguaran

This comment has been minimized.

Show comment
Hide comment
@guilleiguaran

guilleiguaran Feb 20, 2013

Member

Thanks!!!

Member

guilleiguaran commented on 6871bd9 Feb 20, 2013

Thanks!!!

@hoffm

This comment has been minimized.

Show comment
Hide comment
@hoffm

hoffm Feb 20, 2013

Contributor

Sorry, all. I've made some git mistakes here. I'm quite embarrassed. I think the best approach is for me to close this request and open a fresh one. I realize this is not ideal because the discussions won't be connect. My apologies!

Contributor

hoffm commented Feb 20, 2013

Sorry, all. I've made some git mistakes here. I'm quite embarrassed. I think the best approach is for me to close this request and open a fresh one. I realize this is not ideal because the discussions won't be connect. My apologies!

@senny

This comment has been minimized.

Show comment
Hide comment
@senny

senny Feb 21, 2013

Member

@hoffm you don't need to close a PR. you can always fix your git mistakes and then force push the new version onto the branch. This will automatically update the PR.

Member

senny commented Feb 21, 2013

@hoffm you don't need to close a PR. you can always fix your git mistakes and then force push the new version onto the branch. This will automatically update the PR.

@hoffm

This comment has been minimized.

Show comment
Hide comment
@hoffm

hoffm Feb 21, 2013

Contributor

@senny I'll certainly do that in the future (and also be more careful not to get into this position). In this case, given that I've already started a new request with the same branch name, I think it's cleaner to transfer discussion over there. Is that cool?

Contributor

hoffm commented Feb 21, 2013

@senny I'll certainly do that in the future (and also be more careful not to get into this position). In this case, given that I've already started a new request with the same branch name, I think it's cleaner to transfer discussion over there. Is that cool?

@senny

This comment has been minimized.

Show comment
Hide comment
@senny

senny Feb 21, 2013

Member

yes that's reasonable, just wanted to let you know that you don't need to close and reopen.

Member

senny commented Feb 21, 2013

yes that's reasonable, just wanted to let you know that you don't need to close and reopen.

@steveklabnik

This comment has been minimized.

Show comment
Hide comment
@steveklabnik
Member

steveklabnik commented on 24b7a76 Feb 21, 2013

👍

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