Skip to content
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

Issue 2802 #2890

Closed
wants to merge 2 commits into from
Closed

Issue 2802 #2890

wants to merge 2 commits into from

Conversation

christianblais
Copy link
Contributor

Added default number values to avoid precision/separator/delimiter bug when locale != :en

With default_locale different from :en and no region locale file, precision, separator and delimiter where not defaulted, resulting in errors such as :

wrong argument type nil (expected Fixnum)

actionpack (3.1.0) lib/action_view/helpers/number_helper.rb:281:in `round'
actionpack (3.1.0) lib/action_view/helpers/number_helper.rb:281:in `number_with_precision'
actionpack (3.1.0) lib/action_view/helpers/number_helper.rb:362:in `number_to_human_size'

@vijaydev
Copy link
Member

vijaydev commented Sep 6, 2011

Needs tests

@spastorino
Copy link
Contributor

Can you test that using i18n master?

@vijaydev
Copy link
Member

cc @spastorino

@rafaelfranca
Copy link
Member

This pull request cannot be automatically merged. Please rebase it against the master. I'll be glad to ask someone from the Core Team to review it.

Thanks.

@rafaelfranca
Copy link
Member

Also we need to fix the others number helpers.

@carlosantoniodasilva
Copy link
Member

I've rebased the patch locally and tested, and it works. What I'm thinking about is that we already have a way to fallback using i18n and config.i18n.fallbacks = true, which could most likely solve the problem by returning the defaults in case none is found.

If that does not solve, I'd propose fallbacking manually to :en, the one we are sure that come with the framework, instead of having constants. I've tested and seems to work fine. Thoughts?

@dui
Copy link

dui commented May 1, 2012

The OP used the same approach as c7e6777 for currencies, which also uses constants rather than :en, so I suppose whichever decision arguments were made then should be valid now.

I also think number_with_precision shouldn't raise regardless of config.i18n.fallbacks = true, just like other translations don't raise if fallbacks is not enabled. And it doesn't make sense to show "translation missing" to a number lacking format. :)

@carlosantoniodasilva
Copy link
Member

Yeah, I've seen that change where they used constants. I don't know which was the decision at that time, but I'll find out :).

Using config.i18n.fallbacks was a possible solution for those currently facing the issue. I agree number helpers shouldn't raise exceptions. My idea was having the framework to fallback to :en locale config instead of using constants - because we can be sure that the framework has such configs in its locale file, instead of the possible duplicated constant. Thanks!

@carlosantoniodasilva
Copy link
Member

Just a quick update, I'm working on a solution on top of this one, that includes the other number helpers.

@rafaelfranca
Copy link
Member

@carlosantoniodasilva great!!!!!!!!!!!!!!!!!! ❤️

carlosantoniodasilva added a commit to carlosantoniodasilva/rails that referenced this pull request May 14, 2012
Action Pack already comes with a default locale fine for :en, that is
always loaded. We can just fallback to this locale for defaults, if
values for the current locale cannot be found.

Closes rails#4420, rails#2802, rails#2890.
carlosantoniodasilva added a commit to carlosantoniodasilva/rails that referenced this pull request Aug 11, 2012
Action Pack already comes with a default locale fine for :en, that is
always loaded. We can just fallback to this locale for defaults, if
values for the current locale cannot be found.

Closes rails#4420, rails#2802, rails#2890.
carlosantoniodasilva added a commit that referenced this pull request Aug 11, 2012
Action Pack already comes with a default locale fine for :en, that is
always loaded. We can just fallback to this locale for defaults, if
values for the current locale cannot be found.

Closes #4420, #2802, #2890.
@carlosantoniodasilva
Copy link
Member

It has been merged to master in 56627b6. Please let us know if you have any other issue after this. Thanks.

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

Successfully merging this pull request may close these issues.

None yet

6 participants