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

Additional Support for Date Time pattern formats #347

Merged
merged 2 commits into from
Mar 3, 2016

Conversation

sachinpali146
Copy link
Contributor

No description provided.

@codecov-io
Copy link

Current coverage is 83.36%

Merging #347 into master will not affect coverage as of 8ea75fb

@@            master    #347   diff @@
======================================
  Files           21      21       
  Stmts         3781    3781       
  Branches         0       0       
  Methods          0       0       
======================================
  Hit           3152    3152       
  Partial          0       0       
  Missed         629     629       

Review entire Coverage Diff as of 8ea75fb

Powered by Codecov. Updated on successful CI builds.

@codecov-io
Copy link

Current coverage is 89.67%

Merging #347 into master will increase coverage by +0.01% as of b501a6c

@@            master    #347   diff @@
======================================
  Files           23      23       
  Stmts         3794    3796     +2
  Branches         0       0       
  Methods          0       0       
======================================
+ Hit           3402    3404     +2
  Partial          0       0       
  Missed         392     392       

Review entire Coverage Diff as of b501a6c

Powered by Codecov. Updated on successful CI builds.

@akx
Copy link
Member

akx commented Feb 14, 2016

Can you remove the merge commit from this PR? It should be as easy as

git remote add upstream https://github.com/python-babel/babel.git
git fetch --all
git rebase upstream/master

width = {3: 'abbreviated', 4: 'wide', 5: 'narrow'}[num]
context = {3: 'format', 4: 'format', 5: 'stand-alone'}[num]
width = {3: 'abbreviated', 4: 'wide', 5: 'narrow', 6: 'short'}[num]
if char == 'c':
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Can you add some sort of reference about the semantics of lower-case c using stand-alone context? It doesn't seem immediately obvious, esp. considering this is a format function...

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@akx I screwed up the commit once again. Please help

@sachinpali146
Copy link
Contributor Author

I will work on it on Monday

@akx
Copy link
Member

akx commented Feb 25, 2016

@sachinpali146:

doing

git fetch --all
git rebase upstream/master

seems to untangle the history enough.

You can then do git reset upstream/master to return to a state where all of your changes exist, but have not been committed yet.

@sachinpali146
Copy link
Contributor Author

@akx Made some changes, please check

@@ -155,6 +160,28 @@ def test_local_day_of_week_standalone(self):
fmt = dates.DateTimeFormat(d, locale='bn_BD')
self.assertEqual('4', fmt['c']) # friday is first day of week

def test_pattern_day_of_week(self):
dt = datetime(2016,2,6)
fmt = dates.DateTimeFormat(dt, locale='en_US')
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'd love to see the "local day of week" formats (e and c) being tested with a locale that has another start-of-week-day than en_US's Sunday.

Otherwise, this PR looks very good!

Added test cases and additional pattern for quarter format
Added test cases and additional pattern for Weekday format
@akx
Copy link
Member

akx commented Mar 3, 2016

Nice work! Thanks @sachinpali146 :)

akx added a commit that referenced this pull request Mar 3, 2016
Additional Support for Date Time pattern formats
@akx akx merged commit 170187e into python-babel:master Mar 3, 2016
@pyup-bot pyup-bot mentioned this pull request Apr 11, 2017
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants