Get digits and roundings from cldr #82

Merged
merged 14 commits into from Nov 13, 2012

Projects

None yet

3 participants

@tanin47
  • Using currency data from CLDR (code, precision, rounding, and name)
  • The currency data is now locale-specific. If a currency record of a locale doesn't exist, it falls back to root
  • Still use countries from the previous currencies.yml for translating from a country name to a currency code
@tanin47

@camertron I've finished it, though I have to use my version of ruby-cldr. The architecture or ruby-cldr doesn't support exporting things without a locale. The currency's precision and rounding are not locale-specific.

I already sent the pull request to ruby-cldr. I hope my version will get merged!

If you have time, please feel free to take a look at my pull request to ruby-cldr. Maybe you have a better architecture in mind. Thanks.

@KL-7 KL-7 commented on an outdated diff Nov 2, 2012
...twitter_cldr/formatters/numbers/currency_formatter.rb
end
- def default_format_options_for(number)
- precision = precision_from(number)
- { :precision => precision == 0 ? DEFAULT_PRECISION : precision }
+ private
+ def resource(code)
+ @resource ||= TwitterCldr.get_resource(:shared, :currency_digits_and_rounding)
+ (@resource[code.to_sym] || @resource[:DEFAULT])
@KL-7
KL-7 Nov 2, 2012

It's up to you if you prefer it that way, but in my opinion parentheses are needless in cases like that.

@KL-7 KL-7 commented on an outdated diff Nov 2, 2012
Gemfile
@@ -8,7 +8,7 @@ end
group :development do
gem 'nokogiri'
- gem 'ruby-cldr', :github => 'svenfuchs/ruby-cldr'
+ gem 'ruby-cldr', :github => 'tanin47/ruby-cldr', :ref => "aabe67a0da625b014bf4af72f41326cb586d0bd2"
@KL-7
KL-7 Nov 2, 2012

Looks like this revision is no longer in the master branch of your fork. Its master points to 5f1733b82c065ba516e4d97eefcdde8a8427f941 now.

@KL-7 KL-7 commented on the diff Nov 2, 2012
lib/twitter_cldr/resources/locales_resources_importer.rb
@@ -65,7 +70,7 @@ def deep_symbolize(component, locale, path)
end
def process_plurals(component, locale, path)
- return unless component == 'plurals'
+ return unless component == 'Plurals'
@KL-7
KL-7 Nov 2, 2012

Why capitalized again? I believe it was fixed in ruby-cldr to be consistent with other components, e.g., numbers. Same with calendars below. I think you should remove this capitalization both here and in your ruby-cldr fork.

@tanin47
tanin47 Nov 2, 2012

There was a bug. Data.components, used as defaults when --components is not specifed, is in Camel-case, but the options for --components should be entered in Snake-case.

Therefore, if we ran bundle exec thor cldr:export, plurals.xml wasn't exported.

So, right now, everything entered is camelized in order to fix this bug. (I should explain this in that pull request)

@KL-7
KL-7 Nov 2, 2012

Ah, I see. Nice catch! In that case the only thing I'd suggest to do is to update README, because it contains misleading example with lowercase components names.

@tanin47
tanin47 Nov 2, 2012

Now either bundle exec thor cldr:export --components Plurals CurrencyDigitsAndRounding or bundle exec thor cldr:export --components plurals currency_digits_and_rounding works.

All the comments are very useful. Appreciate your time. I'll get them fixed soon!

@camertron
camertron Nov 11, 2012

Interestingly, running bundle exec rake update:locales_resources now outputs a bunch of plurals.rb files in each locale resource directory - is this the reason why?

@KL-7 KL-7 commented on an outdated diff Nov 2, 2012
lib/twitter_cldr/shared/currencies.rb
end
- def currency_codes
- resource.values.map { |data| data[:code] }
+ def currency_codes(locale = :en)
+ resource(locale).keys.map{|c| c.to_s}
@KL-7
KL-7 Nov 2, 2012

Afaik most rubists agree (search for {) that having spaces around braces makes code much easier to read. Besides, here you can easily use map(&:to_s).

@KL-7 KL-7 commented on an outdated diff Nov 2, 2012
lib/twitter_cldr/shared/currencies.rb
end
- def for_country(country_name)
- resource[country_name.to_sym]
+ def for_country(country_name, locale = :en)
+ Kernel.warn("Currencies#for_country will be deprecated. Please stop using it.")
+ return nil if !resource_countries[country_name.to_sym]
@KL-7
KL-7 Nov 2, 2012

There's no need to specify nil explicitly – return without a value will return nil anyway. Besides, unless is a bit more expressive than if !something, and in a two-lines method I'd avoid explicit return altogether and rather move if-condition to the end of the last line.

@KL-7 KL-7 commented on an outdated diff Nov 2, 2012
lib/twitter_cldr/resources/locales_resources_importer.rb
add_buddhist_calendar(component, locale, path)
process_plurals(component, locale, path)
deep_symbolize(component, locale, path)
end
+ Cldr::Export.export(:components => ["currency_digits_and_rounding"], :target => File.join(@output_path, 'shared')) do |component, locale, path|
+ deep_symbolize(component, locale, path)
+ end
+
+
@KL-7
KL-7 Nov 2, 2012

EmptyLinesOverflow!

@camertron camertron commented on an outdated diff Nov 2, 2012
lib/twitter_cldr/shared/currencies.rb
def countries
- resource.keys.map(&:to_s)
+ Kernel.warn("Currencies#countries will be deprecated. Please stop using it.")
@camertron
camertron Nov 2, 2012

It might be nice to tell everyone what the replacement is for Currencies#countries.

@camertron camertron commented on the diff Nov 2, 2012
spec/shared/currencies_spec.rb
data.should include(
- :code => "PEN",
- :currency => "Nuevo Sol",
- :symbol => "S/."
+ :currency => :PEN,
+ :name => "Peruvian nuevo sol",
+ :symbol => nil
@camertron
camertron Nov 2, 2012

This is kinda weird. I know the the currency symbol for the Nuevo Sol is "S/." - is that not in the CLDR repo?

@tanin47
tanin47 Nov 2, 2012

I see it now. The currency symbol is per locale, and the locale that uses S/. is es_PE.

But in twitter-cldr-rb, we don't have es_PE, we only have es.

This currency symbol per locale is really new to me. I never knew that before!

@camertron

Hey @tanin47, it looks like the Travis build for this branch is failing. I think it's just because you've got that literal SHA1 in the Gemfile.

@tanin47

Actually, all the test cases are not green. If I run with Ruby 1.9.3, I got this error:

[tw-mba-tnakorn twitter-cldr-rb (get_digits_and_roundings_from_cldr)]$ rvm use 1.9.3
Using /opt/twitter/rvm/gems/ruby-1.9.3-p125
[tw-mba-tnakorn twitter-cldr-rb (get_digits_and_roundings_from_cldr)]$ bundle exec rspec spec/*
/opt/twitter/rvm/rubies/ruby-1.9.3-p125/lib/ruby/1.9.1/yaml.rb:37:in `yamler=': undefined method `syck_to_yaml' for class `Object' (NameError)
    from /opt/twitter/rvm/rubies/ruby-1.9.3-p125/lib/ruby/1.9.1/yaml.rb:33:in `class_eval'
    from /opt/twitter/rvm/rubies/ruby-1.9.3-p125/lib/ruby/1.9.1/yaml.rb:33:in `yamler='
    from /Users/tnanakorn/workspace/twitter-cldr-rb/spec/utils/yaml/yaml_spec.rb:33:in `<top (required)>'
    from /opt/twitter/rvm/gems/ruby-1.9.3-p125/gems/rspec-core-2.11.1/lib/rspec/core/configuration.rb:780:in `load'
    from /opt/twitter/rvm/gems/ruby-1.9.3-p125/gems/rspec-core-2.11.1/lib/rspec/core/configuration.rb:780:in `block in load_spec_files'
    from /opt/twitter/rvm/gems/ruby-1.9.3-p125/gems/rspec-core-2.11.1/lib/rspec/core/configuration.rb:780:in `map'
    from /opt/twitter/rvm/gems/ruby-1.9.3-p125/gems/rspec-core-2.11.1/lib/rspec/core/configuration.rb:780:in `load_spec_files'
    from /opt/twitter/rvm/gems/ruby-1.9.3-p125/gems/rspec-core-2.11.1/lib/rspec/core/command_line.rb:22:in `run'
    from /opt/twitter/rvm/gems/ruby-1.9.3-p125/gems/rspec-core-2.11.1/lib/rspec/core/runner.rb:69:in `run'
    from /opt/twitter/rvm/gems/ruby-1.9.3-p125/gems/rspec-core-2.11.1/lib/rspec/core/runner.rb:8:in `block in autorun'

Is this normal?

Everything is ok for Ruby 1.8.7 though.

@camertron

Hey @tanin47,

Yeah, that happens occasionally if you don't build Ruby with YAML support :( We should probably look into that and fix it so it doesn't raise an exception.

Is this feature ready to merge?

@tanin47

@camertron All the tests pass. I believe it is ready :)

@KL-7

For some reason I don't get this error on my local 1.9.3, but it happens on Travis now =(

@tanin47

@KL-7 I got that error when I use 1.9.3, but everything works fine when I use 1.8.7.

Cametron said that because 1.9.3 is not installed with YAML. Maybe that is the case on Travis

@camertron

Hmm... Travis should have Rubies built with YAML support, this is kind of weird. I don't get this error on master or for the other branches I'm working on. That being said, the issue looks like it can be solved with a simple require. See what happens next time if you change the first few lines in spec/utils/yaml/yaml_spec.rb to this:

if YAML.const_defined?(:ENGINE)
  require 'syck'  # this is new, seems to fix things
  YAML::ENGINE.yamler = 'syck'
end
@camertron camertron merged commit 5a0749e into twitter:master Nov 13, 2012

1 check passed

Details default The Travis build passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment