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

Implement date interval formatting #316

Merged
merged 8 commits into from Jan 23, 2016
Merged

Implement date interval formatting #316

merged 8 commits into from Jan 23, 2016

Conversation

@akx
Copy link
Member

@akx akx commented Jan 4, 2016

Nb: There are some plumbing commits at the top -- the changes there are required by the actual interval code.

Fixes #276

@akx akx added this to the Babel 2.3 milestone Jan 4, 2016
@gitmate-bot gitmate-bot added the size/XL label Jan 4, 2016
@akx akx force-pushed the akx:ivals branch from 40a1343 to 3560b00 Jan 4, 2016
@codecov-io
Copy link

@codecov-io codecov-io commented Jan 4, 2016

Current coverage is 89.40%

Merging #316 into master will increase coverage by +1.02% as of afd09f0

@@            master   #316   diff @@
=====================================
  Files           23     23       
  Stmts         3626   3736   +110
  Branches         0      0       
  Methods          0      0       
=====================================
+ Hit           3205   3340   +135
  Partial          0      0       
+ Missed         421    396    -25

Review entire Coverage Diff as of afd09f0

Powered by Codecov. Updated on successful CI builds.

@@ -16,6 +16,9 @@
import os
import re
import sys

from babel.dates import split_interval_pattern

This comment has been minimized.

@etanol

etanol Jan 7, 2016
Contributor

This must go below the sys.path manipulation, i.e., together with the other babel imputs. Otherwise, you may import a different Babel version.

This comment has been minimized.

@akx

akx Jan 8, 2016
Author Member

Oop, good catch. I just used PyCharm's automatic import generation, so I didn't even notice...

@@ -694,7 +718,7 @@ def format_time(time=None, format='medium', tzinfo=None, locale=LC_TIME):
return parse_pattern(format).apply(time, locale)


def format_skeleton(skeleton, datetime=None, tzinfo=None, locale=LC_TIME):
def format_skeleton(skeleton, datetime=None, tzinfo=None, fuzzy=True, locale=LC_TIME):

This comment has been minimized.

@etanol

etanol Jan 7, 2016
Contributor

More tests needed, e.g., with fuzzy == False and tzinfo != None. Use the tests package if you don't want to pollute the docstring.

This comment has been minimized.

@akx

akx Jan 8, 2016
Author Member

Those are now added in a new test case file.

)


def format_interval(start, end, skeleton, tzinfo=None, fuzzy=True, locale=LC_TIME):

This comment has been minimized.

@etanol

etanol Jan 7, 2016
Contributor

Again, more tests needed, at least to cover the tzinfo and fuzzy parameters.

@etanol
Copy link
Contributor

@etanol etanol commented Jan 7, 2016

Looks good. However, for the amount of newly added code, I see few test cases.

@akx akx force-pushed the akx:ivals branch from 3560b00 to becebe9 Jan 8, 2016
@akx
Copy link
Member Author

@akx akx commented Jan 8, 2016

Thanks for prodding me to add more tests, @etanol. Found a few corner-case bugs too, which are now fixed.

@akx akx force-pushed the akx:ivals branch 2 times, most recently from 8bdb885 to 26c13df Jan 8, 2016
'jyMMd'
>>> bool(match_skeleton('yMMd', ('qyMMd',), allow_different_fields=False))
False

This comment has been minimized.

@etanol

etanol Jan 9, 2016
Contributor

If this returns None just change False to None in the docstring. The bool() wrapping doesn't add any real value here.

This comment has been minimized.

@akx

akx Jan 11, 2016
Author Member

An explicit None there won't work:

>>> match_skeleton('yMMd', ('qyMMd',), allow_different_fields=False)
Expected: None
Got nothing

but leaving the retval out entirely works, so I've done that in a forthcoming revision of the PR.

def test_format_interval_no_difference():
t1 = TEST_DT
t2 = t1 + datetime.timedelta(minutes=8)
assert dates.format_interval(t1, t2, "yMd", fuzzy=False, locale="fi") == "8.1.2016"

This comment has been minimized.

@etanol

etanol Jan 9, 2016
Contributor

I still don't see any tests for the tzinfo parameter of the format_interval function.

This comment has been minimized.

@akx

akx Jan 11, 2016
Author Member

Test added (though I don't think any Finn would ever format a timezoned time like that, but that's not the point here) :).

@akx akx force-pushed the akx:ivals branch 4 times, most recently from ea7a817 to c1ec13f Jan 11, 2016
@akx
Copy link
Member Author

@akx akx commented Jan 12, 2016

Comments heeded, fixes folded in and branch rebased.

Since Github predictably threw away the commit-related comments, here:

image

@akx
Copy link
Member Author

@akx akx commented Jan 12, 2016

@sils1297, @etanol: This is ready for review again.

dt = datetime.utcnow()
else:
dt = dt.replace(tzinfo=None)
dt = _get_datetime(dt).replace(tzinfo=None)

This comment has been minimized.

@etanol

etanol Jan 13, 2016
Contributor

Why does Codecov show this line as not covered? It should be

elif char == 'H':
return self.value.hour
elif char == 'h':
return (self.value.hour % 12 or 12)

This comment has been minimized.

@etanol

etanol Jan 13, 2016
Contributor

Case not covered.

This comment has been minimized.

@akx

akx Jan 13, 2016
Author Member

Added test_format_interval_12_hour().

This comment has been minimized.

@jtwang

jtwang Jan 22, 2016
Contributor

It would be kind of neat to see a test method specifically for extract, but no big deal, feel free to ignore.

options = sorted(option for option in options if option)

if 'z' in skeleton and not any('z' in option for option in options):
skeleton = skeleton.replace('z', 'v')

This comment has been minimized.

@etanol

etanol Jan 13, 2016
Contributor

This is also not covered. Would it be interesting to add a case for this line?

This comment has been minimized.

@akx

akx Jan 13, 2016
Author Member

Added doctest: match_skeleton('hmz', ('hmv',))

@akx akx force-pushed the akx:ivals branch from c1ec13f to 5d1472e Jan 13, 2016
@akx
Copy link
Member Author

@akx akx commented Jan 13, 2016

Coverage added. :)

@@ -38,6 +38,106 @@
time_ = time


def _get_dt_and_tzinfo(dt_or_tzinfo):

This comment has been minimized.

@jtwang

jtwang Jan 22, 2016
Contributor

This refactor is great!

@akx akx force-pushed the akx:ivals branch from 143950d to cd70395 Jan 23, 2016
akx added a commit that referenced this pull request Jan 23, 2016
Implement date interval formatting
@akx akx merged commit 0d78912 into python-babel:master Jan 23, 2016
5 of 6 checks passed
5 of 6 checks passed
review/gitmate/manual This commit needs review.
Details
codecov/patch 97.00% of diff hit (target 80.00%)
Details
codecov/project 89.40% (+1.02%) compared to 459d30f at 88.38%
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
@akx akx deleted the akx:ivals branch Jan 23, 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

5 participants