Fix #325 Support more format for timezone #338

sachinpali146 · 2016-02-01T21:55:38Z

Added the Support for timezone format 'ZZZZZ' and 'X'

codecov-io · 2016-02-01T21:55:41Z

Current coverage is `83.36%`

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

@@            master    #338   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 · 2016-02-01T22:15:15Z

Current coverage is `89.56%`

Merging #338 into master will increase coverage by +0.09% as of f6a1cb5

@@            master    #338   diff @@
======================================
  Files           23      23       
  Stmts         3752    3785    +33
  Branches         0       0       
  Methods          0       0       
======================================
+ Hit           3357    3390    +33
  Partial          0       0       
  Missed         395     395

Review entire Coverage Diff as of f6a1cb5

Powered by Codecov. Updated on successful CI builds.

akx · 2016-02-01T22:33:37Z

babel/dates.py

@@ -395,7 +395,9 @@ def get_timezone_gmt(datetime=None, width='long', locale=LC_TIME):

    :param datetime: the ``datetime`` object; if `None`, the current date and
                     time in UTC is used
-    :param width: either "long" or "short"
+    Added new parameter for width to resolve bug #325


This doesn't need to be in the docstring.

akx · 2016-02-01T22:44:58Z

@sachinpali146 Thank you for the patch!

However please review the code formatting comments made by @gitmate-bot, above, and my inline comments about the code itself.

In addition, I don't see test cases for the new formatting; could you please add some? Extra credit if some tests match the examples in the TR35 (such as ZZZZZ formatting the America/Los Angeles timezone as -08:00).

sachinpali146 · 2016-02-02T06:07:44Z

@akx Thanks for the suggestions, I will work on them. I am working on open source project for first time so I will be needing some guidance.

akx · 2016-02-02T22:34:09Z

babel/dates.py

@@ -1282,6 +1284,16 @@ def format_timezone(self, char, num):
                return get_timezone_name(self.value.tzinfo, width,
                                         uncommon=True, locale=self.locale)
            return get_timezone_location(self.value.tzinfo, locale=self.locale)
+        # Included additional elif condition to add support for 'X' in timezone format
+        elif char in "xX":
+            if get_timezone_gmt(self.value, width='short', locale=self.locale)=='+0000' and char == 'X':


Hmm... It still feels kind of wasteful to have get_timezone_gmt go through actually formatting the date just so its return value could be turned into a zero :)

I think it might be better to actually add an argument to get_timezone_gmt, something like return_z (optional, false by default), which, when set to True, would cause its logic to short-circuit to returning "Z" directly after it has calculated that the offset would in fact be zero.

What do you think?

Yes, initially I was also thinking to add some argument in get_timezone_gmt function, should I go ahead and add the functionality

sachinpali146 · 2016-02-04T09:34:36Z

@akx Can you please help me in understanding how codecov works and why commits are failing.

akx · 2016-02-04T16:54:05Z

@sachinpali146 Sure.

Codecov measures the code coverage for the tests that are being run for the code.

In this particular case,

codecov/patch - 66% of diff hit (target 80%)

means that of the lines you have changed, only 66% are covered by the test suite.

Likewise,

codecov/project - 89.03% (-0.44%) compared to 89.47%

means that while the complete test coverage of the project had been 89.47% before this PR, with the added code it's dropped down to 89.03%.

What this means, all in all, is that there's new code that is left untested, and the way to fix it is to add tests that cover all of the new behavior.

So -- could you write tests to cover the new behavior you've implemented?

sachinpali146 · 2016-02-04T21:57:21Z

@akx Thanks for the guidance about code coverage. I had added the test cases for the patch and it covered 100% patch code and also increased the project code coverage :)

akx · 2016-02-05T06:05:35Z

babel/dates.py

    :param locale: the `Locale` object, or a locale string
+    :param return_z: True or False; Function returns indicator "Z"
+                     when local time offset is 0
+    :param x_format: True or False; Function returns The ISO8601 basic format


The z flag is fine, but I think this flag needs to be rethought, since it looks like it only affects one situation anyway.

Maybe make this another width? iso8601_short, for instance?

So iso8601 would always return the +HH:MM format, and iso8601_short would return either +HH or +HH:MM?

akx · 2016-02-05T06:08:49Z

Very impressive work with the tests, good job! 🎉

There's still a couple things I'd like to address; see the inline comments, and also pay attention to Gitmate's stylistic complaints. (You seem to be missing a couple whitespaces after commas and so on. :) )

akx · 2016-02-05T12:09:40Z

babel/dates.py

@@ -437,7 +454,10 @@ def get_timezone_location(dt_or_tzinfo=None, locale=LC_TIME):
                         the timezone; if `None`, the current date and time in
                         UTC is assumed
    :param locale: the `Locale` object, or a locale string
-    :return: the localized timezone name using location format
+    :param return_city: True or False


The parameter description should include what it does too. Maybe something like

Return only the exemplar city for the time zone when true

?

I did added that info but at wrong location, Should I move it up?
:param return_city: True or False
:return: the localized timezone name using location format;
if return_city is True then return The exemplar city (location) for the time zone

akx · 2016-02-05T12:19:55Z

@sachinpali146 The PR is starting to look very good now!

Could you just squash the changes into a couple commits with messages following the Writing Good Commits rules? Maybe

one commit adding the new features to get_timezone_gmt, get_timezone_location and get_timezone_name (with the respective tests)
one commit adding the new format characters to the formatter class (with the respective tests)

If you're unfamiliar with rewriting commits like this, try the Git Book's Interactive Staging tutorial. I'd personally do something like (assuming we're starting from this branch's HEAD and a remote called upstream refers to this repository):

$ git fetch --all # (get all changes from all remotes)
$ git rebase upstream/master # (rebase your changes on top of the current master; there should not be any conflicts to resolve)
$ git reset upstream/master # (reset your repo to a state where your changes are intact in the working tree but the commits aren't there)
$ git add -p # (include the first set of changes)
$ git commit # (write a fantastic commit message)
$ git add -u # (to add the rest of the updated files)
$ git commit # (write another fantastic message)
$ git push -f # (force-push to your PR branch)

Added argument 'return_z' and two values 'iso8601' and 'iso8601_short' in argument width in get_timezone_gmt(), 'return_city' argument is add in get_timezone_location() and 'return_zone' in get_timezone_name() so we can implement iso8601 timezone patterns.

python-babel#325

akx · 2016-02-05T16:27:28Z

ack 949e4cb, b759308

akx · 2016-02-05T16:28:17Z

Thank you very much for the contributions, @sachinpali146! And thanks for putting up with my nitpicking. This is an exemplary patch. :)

Merging this -- have a great weekend!

Fix #325 Support more format for timezone

sachinpali146 · 2016-02-05T17:38:02Z

Thank you @akx for your patience, I had learned lot of thing by working on this PR and your continuous guidance. It is your nitpicking which has helped me to become open source society contributor :)

gitmate-bot added the size/S label Feb 1, 2016

akx reviewed Feb 1, 2016
View reviewed changes

akx reviewed Feb 2, 2016
View reviewed changes

akx assigned akx Feb 3, 2016

gitmate-bot added size/L and removed size/S labels Feb 3, 2016

akx reviewed Feb 5, 2016
View reviewed changes

dates: Add iso8601 pattern timezone Format

b759308

python-babel#325

sachinpali146 force-pushed the master branch from 0fb90d9 to b759308 Compare February 5, 2016 15:52

akx added a commit that referenced this pull request Feb 5, 2016

Merge pull request #338 from sachinpali146/master

dd467ca

Fix #325 Support more format for timezone

akx merged commit dd467ca into python-babel:master Feb 5, 2016

This was referenced Oct 18, 2016

Pin babel to latest version 2.3.4 MinchinWeb/colourettu#24

Closed

Initial Update wooyek/flask-social-blueprint#25

Merged

pyup-bot mentioned this pull request Jan 31, 2017

Update babel to 2.3.4 mkurek/ralph#28

Open

pyup-bot mentioned this pull request Apr 11, 2017

Initial Update do3cc/blog#3

Closed

pyup-bot mentioned this pull request Apr 27, 2017

Pin babel to latest version 2.4.0 osteele/assignment-dashboard#24

Open

pyup-bot mentioned this pull request May 12, 2017

Initial Update tantale/intranet#43

Closed

pyup-bot mentioned this pull request Jul 4, 2017

Initial Update pelson/pyggybank#2

Merged

This was referenced Aug 17, 2017

Pin babel to latest version 2.4.0 springermac/pyinstaller#67

Merged

Pin babel to latest version 2.5.0 osteele/assignment-dashboard#51

Open

This was referenced Aug 26, 2017

Pin babel to latest version 2.5.0 nicfit/nicfit.py#129

Merged

Initial Update oddbird/oddsite#127

Closed

pyup-bot mentioned this pull request Sep 4, 2017

Scheduled weekly dependency update for week 36 oddbird/oddsite#128

Merged

pyup-bot mentioned this pull request Sep 14, 2017

Pin babel to latest version 2.5.1 osteele/assignment-dashboard#56

Open

pyup-bot mentioned this pull request Oct 17, 2017

Initial Update tracim/tracim#373

Closed

pyup-bot mentioned this pull request Nov 3, 2017

Update babel to 2.5.1 tracim/tracim#424

Closed

This was referenced Jan 15, 2018

Pin babel to latest version 2.5.2 osteele/assignment-dashboard#76

Open

Pin babel to latest version 2.5.3 osteele/assignment-dashboard#78

Open

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Fix #325 Support more format for timezone #338

Fix #325 Support more format for timezone #338

sachinpali146 commented Feb 1, 2016

codecov-io commented Feb 1, 2016

codecov-io commented Feb 1, 2016

akx Feb 1, 2016

akx commented Feb 1, 2016

sachinpali146 commented Feb 2, 2016

akx Feb 2, 2016

sachinpali146 Feb 3, 2016

sachinpali146 commented Feb 4, 2016

akx commented Feb 4, 2016

sachinpali146 commented Feb 4, 2016

akx Feb 5, 2016

akx commented Feb 5, 2016

akx Feb 5, 2016

sachinpali146 Feb 5, 2016

akx commented Feb 5, 2016

akx commented Feb 5, 2016

akx commented Feb 5, 2016

sachinpali146 commented Feb 5, 2016

Fix #325 Support more format for timezone #338

Fix #325 Support more format for timezone #338

Conversation

sachinpali146 commented Feb 1, 2016

codecov-io commented Feb 1, 2016

Current coverage is 83.36%

codecov-io commented Feb 1, 2016

Current coverage is 89.56%

akx Feb 1, 2016

Choose a reason for hiding this comment

akx commented Feb 1, 2016

sachinpali146 commented Feb 2, 2016

akx Feb 2, 2016

Choose a reason for hiding this comment

sachinpali146 Feb 3, 2016

Choose a reason for hiding this comment

sachinpali146 commented Feb 4, 2016

akx commented Feb 4, 2016

sachinpali146 commented Feb 4, 2016

akx Feb 5, 2016

Choose a reason for hiding this comment

akx commented Feb 5, 2016

akx Feb 5, 2016

Choose a reason for hiding this comment

sachinpali146 Feb 5, 2016

Choose a reason for hiding this comment

akx commented Feb 5, 2016

akx commented Feb 5, 2016

akx commented Feb 5, 2016

sachinpali146 commented Feb 5, 2016

Current coverage is `83.36%`

Current coverage is `89.56%`