Fix the loading of the currency format types #203

etanol · 2015-08-16T09:33:48Z

In order to fix #201, it was necessary to properly interpret the type attribute of the currencyFormat elements in CLDR data. It apparently changed between version 23 and 26.

codecov-io · 2015-08-16T09:40:27Z

Current coverage is `83.11%`

Merging #203 into master will increase coverage by +0.04% as of 14f241c

@@            master    #203   diff @@
======================================
  Files           21      21       
  Stmts         3780    3784     +4
  Branches         0       0       
  Methods          0       0       
======================================
+ Hit           3140    3145     +5
  Partial          0       0       
+ Missed         640     639     -1

Review entire Coverage Diff as of 14f241c

Powered by Codecov. Updated on successful CI builds.

etanol · 2015-08-16T09:45:46Z

The decrease in overall coverage is due to a change in a doctest. Maybe codecov is not running them?

sils · 2015-08-16T09:49:30Z

@etanol tests are run by travis using pytest, if doctests aren't run that's a bug but IIRC they are run.

etanol · 2015-08-16T09:52:30Z

@sils1297 Then, how can I avoid this coverage decrease?

sils · 2015-08-16T10:04:00Z

@etanol you should be able to reproduce the coverage report with make test-cov after pip install pytest-cov. Either doctests aren't executed, should be easy to test locally, or your tests are insufficient. In the first case, please file a bug and refer to it but IIRC pytest should do that.

etanol · 2015-08-18T18:42:00Z

There seems to be a problem with the process environment on Windows for Python 2.7. The function babel.core.default_locale returns None when there is no system locale information available from the environment variables (LANGUAGE, LANG, LC_*).

I'm going to change the doctest so this doesn't fail. However, maybe some review needs to be performed on the AppVeyor configuration, since all Python 3 setups appear to work fine.

erickwilder · 2015-08-24T16:48:36Z

tests/test_numbers.py

+            == u'$1,099.98')
+
+
+def test_format_currency_format_type():


I'm not a big fan of using multiple assertions like that in a single test case, I would write 3 test cases:

test_format_current_with_standard_format_type

test_format_current_with_accounting_format_type

test_format_current_with_invalid_format_type

Just personal taste (this is not a blocker to approve this PR. 😄 )

Duly noted. I'll keep this in mind for future PRs.

erickwilder · 2015-08-24T16:50:05Z

@sils1297 this looks good to me.

erickwilder · 2015-08-25T14:42:48Z

@etanol
Can you add an entry to the CHANGES file, under the upcoming 3.x version just to keep track of changes you've made?

We don't have any formal guideline regarding this, but I think asking PR authors to do this it's the easiest way to keep our change log updated. Thank's

etanol · 2015-08-28T18:53:41Z

@erickwilder Done. I took the chance to include information related to my other PR #180. I hope is not an issue.

sils · 2015-09-07T19:44:46Z

@etanol it seems we are still not capable of maintaining this repository without larger delays for basically everything and you're more of a Babel dev than I am tbh. You do seem to have some kind of interest in Babel, would you be interested in help us reviewing and triaging bugs a bit?

etanol · 2015-09-07T21:27:13Z

@sils1297 Sure, I'll be glad to help. Although my availability is variable, I always try to dedicate a couple of hours a week to free coding.

sils · 2015-09-07T21:41:37Z

@etanol cool, nobody is full time availabe for this one anyway. Whenever your passion takes over feel free to review some "pending review" PRs. I'll try to get you permissions to relabel stuff, we still need @mitsuhiko to do that kind of stuff and it's usually hard to grab him.

erickwilder · 2015-09-09T22:08:33Z

Hi @etanol
Other pull requests were approved and it seems to have caused some conflicts here. Can you rebase this so I can merge it to our master branch?

Sorry for the delay. My daughter was born a couple days ago and I was out of the radar for a while.
Thanks

sils · 2015-09-21T12:28:19Z

@etanol marking this one in-progress as it seems to need a rebase and coverage still complains.

codecov-io · 2015-09-26T15:01:37Z

Current coverage is `83.44%`

Merging #203 into master will increase coverage by +0.04% as of 1a01af8

@@            master    #203   diff @@
======================================
  Files           21      21       
  Stmts         3790    3794     +4
  Branches         0       0       
  Methods          0       0       
======================================
+ Hit           3161    3166     +5
  Partial          0       0       
+ Missed         629     628     -1

Review entire Coverage Diff as of 1a01af8

Powered by Codecov. Updated on successful CI builds.

sils · 2015-09-26T16:09:27Z

@etanol are there any notable changes from the state where @erickwilder approved this? Does it need another full review?

etanol · 2015-09-26T18:28:43Z

@sils1297 I need to fix the CHANGES file, give me a day or two please.

sils · 2015-09-26T18:47:42Z

You may want to merge my PR that sets the version numbers in CHANGES right
in the meantime.
On Sep 26, 2015 8:28 PM, "Isaac Jurado" [email protected] wrote:

@sils1297 https://github.com/sils1297 I need to fix the CHANGES file,
give me a day or two please.

—
Reply to this email directly or view it on GitHub
#203 (comment).

sils · 2015-09-27T12:56:05Z

And well, this needs of course a rebase because of the damn conflicts file :)

The type of the currency format (e.g. "standard", "accounting") was not interpreted correctly from the CLDR data. Now there should not be any currency format identified by "None". Fixes #201

Previous commit introduced an API change by changing the behaviour of the format param, instead this commit adds a format_type param and the documentation and tests to accompany it. Note that currency_formats returns a NumberPattern already, so there is no need to call parse_pattern on the value returned.

Fix the loading of the currency format types

erickwilder reviewed Aug 24, 2015
View reviewed changes

erickwilder added the partial approved label Aug 24, 2015

sils assigned erickwilder Sep 7, 2015

erickwilder added the needs rebase label Sep 9, 2015

etanol unassigned erickwilder Sep 21, 2015

sils added (2) in progress and removed partial approved labels Sep 21, 2015

etanol mentioned this pull request Sep 26, 2015

bump cldr to 27.0.1 to fix some incorrect negative prefix pattern for currency? #165

Closed

sils added (3) pending review and removed (2) in progress needs rebase labels Sep 26, 2015

sils assigned erickwilder Sep 26, 2015

sils added (2) in progress and removed (3) pending review labels Sep 27, 2015

sils added this to the Babel 2.2 milestone Sep 27, 2015

sils assigned etanol and unassigned erickwilder Sep 27, 2015

etanol and others added 3 commits September 27, 2015 15:05

numbers: Properly load and expose currency format types

df676ab

The type of the currency format (e.g. "standard", "accounting") was not interpreted correctly from the CLDR data. Now there should not be any currency format identified by "None". Fixes #201

Update CHANGES and AUTHORS information

f119583

sils added (3) pending review and removed (2) in progress labels Sep 27, 2015

sils added a commit that referenced this pull request Sep 27, 2015

Merge pull request #203 from etanol/currency-format-types

a82d7b0

Fix the loading of the currency format types

sils merged commit a82d7b0 into python-babel:master Sep 27, 2015

sils removed the (3) pending review label Sep 27, 2015

etanol mentioned this pull request Jan 7, 2016

currency_formats format changed between 2.1.x and 2.2 #317

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Fix the loading of the currency format types #203

Fix the loading of the currency format types #203

etanol commented Aug 16, 2015

codecov-io commented Aug 16, 2015

etanol commented Aug 16, 2015

sils commented Aug 16, 2015

etanol commented Aug 16, 2015

sils commented Aug 16, 2015

etanol commented Aug 18, 2015

erickwilder Aug 24, 2015

etanol Aug 24, 2015

erickwilder commented Aug 24, 2015

erickwilder commented Aug 25, 2015

etanol commented Aug 28, 2015

sils commented Sep 7, 2015

etanol commented Sep 7, 2015

sils commented Sep 7, 2015

erickwilder commented Sep 9, 2015

sils commented Sep 21, 2015

codecov-io commented Sep 26, 2015

sils commented Sep 26, 2015

etanol commented Sep 26, 2015

sils commented Sep 26, 2015

sils commented Sep 27, 2015

Fix the loading of the currency format types #203

Fix the loading of the currency format types #203

Conversation

etanol commented Aug 16, 2015

codecov-io commented Aug 16, 2015

Current coverage is 83.11%

etanol commented Aug 16, 2015

sils commented Aug 16, 2015

etanol commented Aug 16, 2015

sils commented Aug 16, 2015

etanol commented Aug 18, 2015

erickwilder Aug 24, 2015

Choose a reason for hiding this comment

etanol Aug 24, 2015

Choose a reason for hiding this comment

erickwilder commented Aug 24, 2015

erickwilder commented Aug 25, 2015

etanol commented Aug 28, 2015

sils commented Sep 7, 2015

etanol commented Sep 7, 2015

sils commented Sep 7, 2015

erickwilder commented Sep 9, 2015

sils commented Sep 21, 2015

codecov-io commented Sep 26, 2015

Current coverage is 83.44%

sils commented Sep 26, 2015

etanol commented Sep 26, 2015

sils commented Sep 26, 2015

sils commented Sep 27, 2015

Current coverage is `83.11%`

Current coverage is `83.44%`