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

Add unit and compound unit formatting #369

Merged
merged 4 commits into from Apr 7, 2016
Merged

Conversation

@akx
Copy link
Member

@akx akx commented Mar 19, 2016

No description provided.

@codecov-io
Copy link

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

Current coverage is 89.90%

Merging #369 into master will increase coverage by +0.15% as of 0a25c9d

@@            master   #369   diff @@
=====================================
  Files           23     24     +1
  Stmts         3854   3921    +67
  Branches         0      0       
  Methods          0      0       
=====================================
+ Hit           3459   3525    +66
  Partial          0      0       
- Missed         395    396     +1

Review entire Coverage Diff as of 0a25c9d

Powered by Codecov. Updated on successful CI builds.


for unit in elem.findall('compoundUnit'):
unit_type = unit.attrib['type']
box = "%s:%s" % (unit_type, unit_length_type)

This comment has been minimized.

@jtwang

jtwang Mar 25, 2016
Contributor

I wonder why the original implementation uses string concatenation instead of nested dicts (eg date formats)

This comment has been minimized.

@akx

akx Mar 30, 2016
Author Member

Commit 5204709 introduced the colons in the first place, but for alternative forms.
Then 9327e08 reused colons/string concat for lengths (misguidedly, perhaps).

I guess "for no particular reason" is the answer :)

for elem in tree.findall('.//units/unitLength'):
unit_length_type = elem.attrib['type']
for unit in elem.findall('unit'):
unit_type = unit.attrib['type']
box = "%s:%s" % (unit_type, unit_length_type)

This comment has been minimized.

@jtwang

jtwang Mar 25, 2016
Contributor

For readability, I'd rename:

  • box -> unit_and_length and
  • pattern_box -> unit_and_length_patterns

Don't care much, but please do at the very least rename

  • pattern_box -> box_patterns
:return: A key to the `unit_patterns` mapping, or None.
"""
locale = Locale.parse(locale)
pattern = "%s:%s" % (unit_id.split(":", 1)[0], length)

This comment has been minimized.

@jtwang

jtwang Mar 25, 2016
Contributor

Maybe call this unit_length_key? Don't really care all that much.

if pattern in locale.unit_patterns:
return pattern
for unit_pattern in sorted(locale.unit_patterns, key=len):
if unit_pattern.endswith(pattern):

This comment has been minimized.

@jtwang

jtwang Mar 25, 2016
Contributor

Hmm.. What's an example that you would expect to satisfy this condition?

I would have expected a fallback condition along the lines of: unit_pattern.startswith(unit_id + ':')

ETA: I see, to allow 'radian' to match 'angle-radian'. I thought it was doing length fallbacks, but it's doing unit fallbacks instead.

@@ -385,6 +384,205 @@ def format_scientific(number, format=None, locale=LC_NUMERIC):
return pattern.apply(number, locale)


def _find_unit_pattern(unit_id, length='long', locale=LC_NUMERIC):

This comment has been minimized.

@jtwang

jtwang Mar 25, 2016
Contributor

Needs more fallback and length testing please. <3

return unit_pattern


def format_unit(value, measurement_unit, length='long', format=None, locale=LC_NUMERIC):

This comment has been minimized.

@jtwang

jtwang Mar 25, 2016
Contributor

Test all kwargs, blah blah blah. ;)

ETA: Nevermind, just saw one below. <3

plural_form = locale.plural_form(value)
if plural_form in unit_patterns:
return unit_patterns[plural_form].format(formatted_value)
# Fall back to a bad representation.

This comment has been minimized.

@jtwang

jtwang Mar 25, 2016
Contributor

Would it be better to fall back to any plural form? eg. if 'many' doesn't exist, fall back to 'one' or 'zero' or 'few' or whatever is there.

If not, I think measurement_unit is pretty user unreadable. I'd prefer to fall back to displayName (which babel doesn't support) or show no unit at all.

This comment has been minimized.

@akx

akx Mar 30, 2016
Author Member

As of CLDR 28, there's actually no situation in which we could reach the fallback, it seems!

For all locales, every plural form/unit pattern combination is currently covered.

denominator_unit_length = _find_unit_pattern(denominator_unit, length, locale)
if not (numerator_unit_length and denominator_unit_length):
return None
# Since compound units are named "speed-kilometer-per-hour:long", we'll have to slice off

This comment has been minimized.

@jtwang

jtwang Mar 25, 2016
Contributor

Another case for switching [unit:length][plural] -> [unit][length][plural] :)

This comment has been minimized.

@akx

akx Mar 30, 2016
Author Member

Sure. That'll have to wait until 3.0 though, since someone might be using the raw data API.

@akx akx force-pushed the akx:unit_format branch from 7da41b1 to c6bd060 Mar 30, 2016
@@ -0,0 +1,239 @@
# -- encoding: UTF-8 --

from babel._compat import Decimal, string_types

This comment has been minimized.

@gitmate-bot

gitmate-bot Mar 30, 2016

This file contains unused source code.

PyUnusedCodeBear, severity NORMAL, section default.

The issue can be fixed by applying the following patch:

--- a/babel/units.py
+++ b/babel/units.py
@@ -1,6 +1,7 @@
 # -- encoding: UTF-8 --

-from babel._compat import Decimal, string_types
+from babel._compat import Decimal
+from babel._compat import string_types
 from babel.core import Locale
 from babel.numbers import format_decimal, LC_NUMERIC
@akx akx force-pushed the akx:unit_format branch from c6bd060 to a7773f0 Mar 30, 2016
akx added 3 commits Mar 30, 2016
(Instead of colon-separated flat dicts.)

This should not be a breaking change, unless someone has used the private
API `l._data["unit_patterns"]`.
* babel.units.format_unit
* babel.units.format_compound_unit
* babel.units.get_unit_name
@akx akx force-pushed the akx:unit_format branch 2 times, most recently to ca9dede Apr 5, 2016
unit_patterns = locale._data["unit_patterns"][q_unit].get(length, {})
formatted_value = format_decimal(value, format, locale)
plural_form = locale.plural_form(value)
if plural_form in unit_patterns:

This comment has been minimized.

@jtwang

jtwang Apr 5, 2016
Contributor

CLDR here casually mentions length fallbacks (at least that's how I interpreted it here). It'd be awesome to see it here, I don't think it would be that difficult to add an extra iteration through a hardcoded, ordered list of length strings. If it's gross, feel free to punt.

http://www.unicode.org/reports/tr35/tr35-general.html#perUnitPatterns

:param locale: the `Locale` object or locale identifier
:return: A key to the `unit_patterns` mapping, or None.
"""
locale = Locale.parse(locale)

This comment has been minimized.

@jtwang

jtwang Apr 5, 2016
Contributor

Did you intend to call this "numerator_form" or "numerator_unit_pattern" instead?

numerator_unit_length = _find_unit_pattern(numerator_unit, locale=locale)
denominator_unit_length = _find_unit_pattern(denominator_unit, locale=locale)
if not (numerator_unit_length and denominator_unit_length):
return None

This comment has been minimized.

@jtwang

jtwang Apr 5, 2016
Contributor

Nit: Maybe mention that the numerator and denominator values we'd have in this case are "length-kilometer" and "duration-hour"

Format a compound number value, i.e. "kilometers per hour" or similar.
Both unit specifiers are optional to allow for formatting of arbitrary values still according
to the locale's general "per" formatting specifier.

This comment has been minimized.

else: # Bare denominator
formatted_denominator = format_decimal(denominator_value, format=format, locale=locale)

per_pattern = locale._data["compound_unit_patterns"].get("per", {}).get(length, "{0}/{1}")

This comment has been minimized.

@jtwang

jtwang Apr 5, 2016
Contributor

Same length fallback comment as for format_unit() above.

@jtwang
Copy link
Contributor

@jtwang jtwang commented Apr 5, 2016

Ow, my head. Yours must hurt more. ;)

@jtwang
Copy link
Contributor

@jtwang jtwang commented Apr 7, 2016

Looks good, push!

@akx
Copy link
Member Author

@akx akx commented Apr 7, 2016

Ignoring the single Travis failure; it's one of those split-second unluckinesses.

@akx akx merged commit 3b6a2e2 into python-babel:master Apr 7, 2016
3 of 7 checks passed
3 of 7 checks passed
codecov/patch CI failed: coverage not measured fully.
Details
codecov/project CI failed: coverage not measured fully.
Details
continuous-integration/travis-ci/pr The Travis CI build failed
Details
review/gitmate/manual This commit needs review.
Details
continuous-integration/appveyor/pr AppVeyor build succeeded
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:unit_format branch Apr 7, 2016
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

4 participants