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

Character order and measurement systems #368

Merged
merged 5 commits into from Apr 5, 2016

Conversation

@akx
Copy link
Member

@akx akx commented Mar 19, 2016

This is a fixed version of @alexbodn 's PR #367.

The get_unit_name API from that PR is intentionally excluded; a format_unit API will be included in a subsequent PR.

@codecov-io
Copy link

@codecov-io codecov-io commented Mar 19, 2016

Current coverage is 89.11%

Merging #368 into master will decrease coverage by -0.64% as of 01c62dc

@@            master    #368   diff @@
======================================
  Files           23      23       
  Stmts         3854    3860     +6
  Branches         0       0       
  Methods          0       0       
======================================
- Hit           3459    3440    -19
  Partial          0       0       
- Missed         395     420    +25

Review entire Coverage Diff as of 01c62dc

Powered by Codecov. Updated on successful CI builds.

@alexbodn
Copy link

@alexbodn alexbodn commented Mar 19, 2016

thank you very much @akx .
the international web sites developers,
and the pint measurement units project
will enjoy these enhancements the moment you will pull them.

if you could, please tell me when to tell pint people to use upstream instead of my babel.

return self._data['measurement_systems']

@property
def character_order(self):

This comment has been minimized.

@jtwang

jtwang Mar 25, 2016
Contributor

This would have been a great use of a Python 3.4 Enum. Just an observation, no need to do anything here. :)

"""
return self._data['character_order']

@property

This comment has been minimized.

@jtwang

jtwang Mar 25, 2016
Contributor

This feels pretty redundant with the method above. I understand the value of providing the CSS-specific value in this package (commonly used), but I don't think it belongs in the Locale class.

We could create helper modules that provide methods like this that transform CLDR values into ones accepted by other systems. Off hand, I can't think of any other ones that might be relevant for CSS (maybe language code?), but, for instance, converting date format strings to JS compatible ones might be useful to someone.

This comment has been minimized.

@akx

akx Mar 30, 2016
Author Member

I don't think this is that much of an evil. After all, ltr and rtl are fairly common terms... However, now that you do mention it, this should really be called text_direction. I shall rename it post-haste.

This comment has been minimized.

@jtwang

jtwang Apr 5, 2016
Contributor

Yikes, good catch. :) What do you think about transforming "left-to-right" to "ltr" (yah, I'm still trying to get rid of one of the two methods :)

This comment has been minimized.

@akx

akx Apr 5, 2016
Author Member

Well, character_order (well, characterOrder, but we're snakes, not camels) is the CLDR name, so I'd like to keep that as-is. Likewise, direction is the CSS property (though I think it's better spelled as text_direction in Babel, just to be explicit).

@@ -422,6 +422,9 @@ def _process_local_datas(sup, srcdir, destdir, force=False, dump_json=False):
parse_unit_patterns(data, tree)
parse_date_fields(data, tree)

for elem in tree.findall('.//layout/orientation/characterOrder'):

This comment has been minimized.

@jtwang

jtwang Mar 25, 2016
Contributor

You should keep using parse wrapper methods! It's much easier to read.

Also, I was going to poke you about calling _should_skip_elem, but none of the elements in cldr 28 are draft, etc, so whatevs. :)

'rtl'
"""
order = self.character_order
return ''.join([word[0] for word in order.split('-')])

This comment has been minimized.

@jtwang

jtwang Mar 25, 2016
Contributor

cough unit test please ;)

This comment has been minimized.

@akx

akx Mar 30, 2016
Author Member

What would you like to see unit tested?

We know empirically (grep | sort | uniq) left-to-right and right-to-left are the only character_orders we'll ever see, so ltr and rtl will also be the only possible directions. (And this is future-proof: if we ever get top-to-bottom or bottom-to-top or upside-down-and-dinky-donky, those will be handled too.)

@@ -480,6 +483,10 @@ def parse_locale_display_names(data, tree):
for listPattern in listType.findall('listPatternPart'):
list_patterns[listPattern.attrib['type']] = _text(listPattern)

measurement_systems = data.setdefault('measurement_systems', {})
for measurement_system in tree.findall('.//measurementSystemNames/measurementSystemName'):
measurement_systems[measurement_system.attrib['type']] = _text(measurement_system)

This comment has been minimized.

@jtwang

jtwang Mar 25, 2016
Contributor

::squints:: I think you can use _import_type_text(measurement_systems, measurement_system).

@akx
Copy link
Member Author

@akx akx commented Mar 30, 2016

Assigned @jtwang for having done initial review on this.

@akx akx force-pushed the akx:char-order-and-measurement-systems branch from e24c15a to f4fae83 Mar 30, 2016
akx and others added 4 commits Mar 30, 2016
* Refactor into a separate function
* Use `_import_type_text` when importing measurement systems
@akx akx force-pushed the akx:char-order-and-measurement-systems branch from f4fae83 to 99dc0c7 Mar 30, 2016
@akx
Copy link
Member Author

@akx akx commented Mar 30, 2016

Disregard the Codecov fail, btw. I think it's being confused again...

@jtwang
Copy link
Contributor

@jtwang jtwang commented Apr 5, 2016

@akx - OK to merge! Just the nit about two methods, ignore if you want. ;)

@akx
Copy link
Member Author

@akx akx commented Apr 5, 2016

Nits acknowledged.

@akx akx merged commit 84415b2 into python-babel:master Apr 5, 2016
5 of 7 checks passed
5 of 7 checks passed
codecov/project 89.11% (-0.64%) compared to 124294a at 89.75%
Details
review/gitmate/manual This commit needs review.
Details
codecov/patch 100.00% of diff hit (target 80.00%)
Details
continuous-integration/appveyor/pr AppVeyor build succeeded
Details
continuous-integration/travis-ci/pr The Travis CI build passed
Details
review/gitmate/commit No issues with this one - go ahead! :)
Details
review/gitmate/pr All is well! :) (0 problems solved)
Details
@akx akx deleted the akx:char-order-and-measurement-systems branch Apr 5, 2016
@akx
Copy link
Member Author

@akx akx commented Apr 8, 2016

Gah, this PR shows up in my list of open PRs even if it's merged.

EDIT: Welp, adding this comment fixed the issue.

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

Successfully merging this pull request may close these issues.

None yet

5 participants
You can’t perform that action at this time.