Add RBNF - Githubissues

twitter / twitter-cldr-js

JavaScript implementation of the ICU (International Components for Unicode) that uses the Common Locale Data Repository to format dates, plurals, and more. Based on twitter-cldr-rb.

Apache License 2.0

346 stars 55 forks source link

Add RBNF #60

Closed arnavk closed 9 years ago

arnavk commented 9 years ago

~~NOTE: Not ready for merging yet. Some things need to be discussed / cleaned up.~~

To facilitate testing RBNF for multiple locales, there is a separate test bundle loaded only during tests. This contains resources for all locales.
[Fixed] However, the RBNF tests only test in the en locale because the NumberFormatter resources are also tied to the locale.
- Suggested workaround: Inject the NumberFormatter symbols for all locales for tests, just like the RBNF test resources.
~~Allowed failures haven't been incorporated yet. I'll do that once we decide the best way to write tests.~~ Skipped the locales with failures for now.
~~Docs aren't ready yet. I'll work on that as the code review takes place.~~
~~Also, the other build files need to be updated before the merge. I'll do that after the rest of the review is over.~~

arnavk commented 9 years ago

Oh no! something seems to be up with the tests. It seems to be fine on my local machine. I'm investigating.

KL-7 commented 9 years ago

I just got the same failure after updating History.txt in master :( Looking into it as well.

KL-7 commented 9 years ago

The build was failing because of a new version of coffee-script-source gem that we indirectly depend on. I froze its version to 1.8.0 in master, we should be good now. The issue is already reported as jashkenas/coffeescript#3829

arnavk commented 9 years ago

@camertron @KL-7 Apart from the issue about the tests taking time, the rest of the PR is ready for merging. The documentation is done and the assets in the build have been updated. Once the tests are sorted out, we can have the final comments and merge :)

KL-7 commented 9 years ago

Overall this looks good to me, leaving detailed review for @camertron. Amazing work, @radzinzki!

camertron commented 9 years ago

Just a few really small remaining comments @radzinzki. Let me know once they've been addressed and I'll merge right away :)

arnavk commented 9 years ago

@camertron updated the PR as per your review. If you're ok with the changes and there are no further comments, then I think we're good to go.