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 support for date-time skeletons #265

Merged
merged 1 commit into from Jan 4, 2016

Conversation

Projects
None yet
7 participants
@mbirtwell
Copy link
Contributor

commented Sep 28, 2015

The skeletons for dates and times are described on
http://cldr.unicode.org/translation/date-time-patterns under
Additional Date-Time Formats. And are useful when you want to some more
control over formatting dates and times but don't want to force all
locales to use the same pattern.

@codecov-io

This comment has been minimized.

Copy link

commented Sep 28, 2015

Current coverage is 84.26%

Merging #265 into master will increase coverage by +0.05% as of 6c99718

@@            master    #265   diff @@
======================================
  Files           22      22       
  Stmts         3770    3776     +6
  Branches         0       0       
  Methods          0       0       
======================================
+ Hit           3175    3182     +7
  Partial          0       0       
+ Missed         595     594     -1

Review entire Coverage Diff as of 6c99718

Powered by Codecov. Updated on successful CI builds.

@etanol

This comment has been minimized.

Copy link
Contributor

commented Sep 28, 2015

Interesting. However, dedicated test code is missing. I know, doctests should cover it but I have a feeling that, if not me, somebody else will ask you for it.

@xmo-odoo

This comment has been minimized.

Copy link
Contributor

commented Oct 15, 2015

I found this while looking into intervals (as they use skeletons), this PR implements basic support for skeletons, but doesn't seem to support skeleton matching (see "matching skeletons" sub-section) and match adjustment, is that normal/intentional?

I also am not sure about the inheritance support: the spec states

dateFormatItems inherit from their parent locale, so the inherited items need to be considered when processing.

which I read as each skeleton (dateFormatItem/@id) not found on the current locale being looked up in the parent locale, is that automatically resolved by babel?

@sils

This comment has been minimized.

Copy link
Member

commented Oct 16, 2015

@etanol uhm I'mnot quite sure I agree with you here, that would mean that we have redundant tests which is nothing but a maintenance overhead, am I wrong?

@etanol

This comment has been minimized.

Copy link
Contributor

commented Oct 16, 2015

@sils1297 Yes, they are an overhead. However, if you check the test suite you will find that there are quite a few tests that are identical to the corresponding docstring. Since I don't know the whole history of Babel, I'm not clear on how this redundancy originated.

@sils

This comment has been minimized.

Copy link
Member

commented Oct 16, 2015

@etanol so we probably want to avoid redundancy from now on and approve PRs that remove identical tests, agreed? Code duplication is bad and doctest is a great invention to reduce testing overhead, we should use that advantage IMO.

@mbirtwell mbirtwell force-pushed the mbirtwell:add-date-time-skeletons branch from 5519960 to 8f301f1 Oct 21, 2015

@mbirtwell

This comment has been minimized.

Copy link
Contributor Author

commented Oct 21, 2015

@sils1297 I've removed the commit which added the extra tests from this PR.

@xmo-odoo I'll be honest I did the minimal here that I needed for my use-case. Which didn't include skeleton-matching although that looks nice I think this is still useful without that. Inheritance however seems to have been handled for me by babel's CLDR import mechanism. e.g. there is no 'hm' skeleton in the en_GB.xml file but the following works:

In [1]: import babel

In [2]: l = babel.Locale.parse('en_GB')

In [3]: l.datetime_skeletons['hm']
Out[3]: <DateTimePattern 'h:mm a'>
@mbirtwell

This comment has been minimized.

Copy link
Contributor Author

commented Oct 22, 2015

In CLDR 26, which babel is currently using there is a partial list of skeletons that appears to get correctly merged with the parent locale (en_US I think in CLDR 26, although I believe that is also changing).

@xmo-odoo

This comment has been minimized.

Copy link
Contributor

commented Oct 22, 2015

@mbirtwell ok, good to know then (I only checked 23.1 — used by the last release — and 28 — latest CLDR — and neither has "partial" skeletons lists)

@akx

This comment has been minimized.

Copy link
Member

commented Jan 3, 2016

@mbirtwell Could you please rebase on top of 2.2.0? One doctest will also need updating based on the current CLDR data.

================================== FAILURES ===================================
_______________ [doctest] babel.core.Locale.datetime_skeletons ________________
EXAMPLE LOCATION UNKNOWN, not showing all tests of that example
??? >>> Locale('fr').datetime_skeletons['MEd']
Expected:
    <DateTimePattern u'E d/M'>
Got:
    <DateTimePattern 'E dd/MM'>

==================== 1 failed, 520 passed in 5.28 seconds =====================

@akx akx added this to the Babel 2.3/3.0 milestone Jan 3, 2016

Add support for date-time skeletons
The skeletons for dates and times are described on
http://cldr.unicode.org/translation/date-time-patterns under
Additional Date-Time Formats. And are useful when you want to some more
control over formatting dates and times but don't want to force all
locales to use the same pattern.

@mbirtwell mbirtwell force-pushed the mbirtwell:add-date-time-skeletons branch from 8f301f1 to 65e20c1 Jan 4, 2016

@gitmate-bot gitmate-bot added the size/M label Jan 4, 2016

@sils sils assigned akx and unassigned etanol Jan 4, 2016

@sils

This comment has been minimized.

Copy link
Member

commented on tests/test_core.py in 65e20c1 Jan 4, 2016

just quickly glancing over here, but aren't those cases covered by the doctest?

This comment has been minimized.

Copy link
Member

replied Jan 4, 2016

The thai locale isn't. I don't think this is a deal-breaker really :)

This comment has been minimized.

Copy link
Member

replied Jan 4, 2016

right. (Just trying to keep redundancy low.)

@akx

This comment has been minimized.

Copy link
Member

commented Jan 4, 2016

Thanks @mbirtwell! 🎉

@akx akx merged commit 3aa3f29 into python-babel:master Jan 4, 2016

5 of 6 checks passed

review/gitmate/manual This commit needs review.
Details
Scrutinizer 1 updated code elements
Details
codecov/project 84.26% (+0.05%) compared to 07aa84f at 84.21%
Details
continuous-integration/appveyor/pr AppVeyor build succeeded
Details
continuous-integration/travis-ci/pr The Travis CI build passed
Details
review/gitmate/commit All is well! :)
Details

@sils sils removed the (3) pending review label Jan 4, 2016

@akx akx added the feature label Jan 4, 2016

@pyup-bot pyup-bot referenced this pull request Jan 6, 2017

Merged

Update babel to 2.3.4 #13

@pyup-bot pyup-bot referenced this pull request Jan 31, 2017

Open

Update babel to 2.3.4 #28

@pyup-bot pyup-bot referenced this pull request Apr 11, 2017

Open

Initial Update #3

@pyup-bot pyup-bot referenced this pull request May 12, 2017

Closed

Initial Update #43

@pyup-bot pyup-bot referenced this pull request Jul 4, 2017

Merged

Initial Update #2

@pyup-bot pyup-bot referenced this pull request Oct 17, 2017

Closed

Initial Update #373

@pyup-bot pyup-bot referenced this pull request Nov 3, 2017

Closed

Update babel to 2.5.1 #424

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.