GV

GeezerVison

The World Wide Web, restored to 800 pixels of glory.

Home|About|Gaming cabinet|Archives|How it works|Starter sites

On this site

  • Sign in
  • ActionsCI/CD and automation
  • CodespacesCloud development environments
  • All Features
  • Documentation
  • Training
  • Certifications
  • Build with Copilot
  • Customize Copilot
  • Integrations
  • Trending repos
  • GitHub Sponsors
  • Developer articles
  • Enterprises

This view

  • Original page
  • New address

GeezerVison is showing github.com — bpo-31800: Support for colon when parsing time offsets by mariocj89 · Pull Request #4015 · python/cpython

Conversation

@mariocj89

@mariocj89 mariocj89 commented Oct 16, 2017 •
edited by abalkin
Loading

Uh oh!

There was an error while loading. Please reload this page.

Copy link
Copy Markdown
Contributor

Add support to strptime to parse time offsets with a colon between the hour and the minutes.

https://bugs.python.org/issue31800

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
All reactions
@the-knights-who-say-ni the-knights-who-say-ni added the CLA signed label Oct 16, 2017
@bedevere-bot bedevere-bot added the awaiting review label Oct 16, 2017
@mariocj89
mariocj89 force-pushed the offset_colon branch from d13d073 to bf46b6a Compare October 17, 2017 23:08
@abalkin abalkin self-assigned this Oct 19, 2017
@mariocj89

mariocj89 commented Oct 21, 2017

Copy link
Copy Markdown
Contributor Author

included the support for 'Z' in 49098f6 happy to remove it or extract it in a different PR.
All reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
@abalkin
abalkin requested a review from vstinner October 21, 2017 20:14
@vstinner
vstinner requested a review from abalkin October 23, 2017 17:22
abalkin
abalkin requested changes Oct 23, 2017
View reviewed changes
Comment thread Doc/library/datetime.rst Outdated

@abalkin abalkin Oct 23, 2017

Copy link
Copy Markdown
Member

There was a problem hiding this comment.

Choose a reason for hiding this comment

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

Please use all caps for "UTC".

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
All reactions
Comment thread Lib/_strptime.py Outdated

@abalkin abalkin Oct 23, 2017

Copy link
Copy Markdown
Member

There was a problem hiding this comment.

Choose a reason for hiding this comment

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

Please add support for sub-minute offsets. See bpo-5288.

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
All reactions
@bedevere-bot

bedevere-bot commented Oct 23, 2017

Copy link
Copy Markdown

A Python core developer has requested some changes be made to your pull request before we can consider merging it. If you could please address their requests along with any other requests in other reviews from core developers that would be appreciated.

Once you have made the requested changes, please leave a comment on this pull request containing the phrase I have made the requested changes; please review again. I will then notify any core developers who have left a review that you're ready for them to take another look at this pull request.
All reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
@bedevere-bot bedevere-bot added awaiting changes and removed awaiting review labels Oct 23, 2017
@mariocj89
mariocj89 force-pushed the offset_colon branch 2 times, most recently from 16227fa to 59af357 Compare October 23, 2017 22:38
mariocj89 and others added 10 commits October 23, 2017 23:57
@mariocj89
Support for colon when parsing time offsets
567666a 
This change adds support to _strptime to parse time offsets with a colon
between the hour and the minutes. This unblocks the parsing of datetime
string representation generated via isoformat together with all string
that have the colon in its offset (which is quite common and part of the
ISO8606)
@mariocj89
Add news entry
0facd08
@mariocj89
Docs for parsing using %z and colons
af68297
@mariocj89
Add support for Z on %z directive
e0e3873
@mariocj89
Update docs to reflect the addition of parsing Z
7c53307
@mariocj89
Add support for parsing seconds
d184e7b
@mariocj89
Add support for microsecond on strptime
3809303
@mariocj89
Add test case with the new format on datetimetester
7548704
@mariocj89
Improve error message on : invalid use
69e655c
@mariocj89
Typo: utc -> UTC
f718b37
@mariocj89
mariocj89 force-pushed the offset_colon branch from 59af357 to f718b37 Compare October 23, 2017 22:59
@mariocj89

mariocj89 commented Oct 23, 2017

Copy link
Copy Markdown
Contributor Author

I have made the requested changes; please review again
All reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
@bedevere-bot bedevere-bot removed the awaiting changes label Oct 23, 2017
@bedevere-bot

bedevere-bot commented Oct 23, 2017

Copy link
Copy Markdown

Thanks for making the requested changes!

@abalkin: please review the changes made to this pull request.
All reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
@bedevere-bot bedevere-bot added the awaiting change review label Oct 23, 2017
@mariocj89

mariocj89 commented Oct 23, 2017 •
edited
Loading

Uh oh!

There was an error while loading. Please reload this page.

Copy link
Copy Markdown
Contributor Author

Added both support for second and microseconds worries me that now the offset can be both a float and an int depending on whether there are microseconds in it though.

Open to suggestions!

Thanks
All reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
@abalkin

abalkin commented Oct 24, 2017

Copy link
Copy Markdown
Member

@mariocj89 - the offset is a timedelta, not int or float. Let me take a look at your changes ...
All reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
abalkin
abalkin reviewed Oct 24, 2017
View reviewed changes
Comment thread Lib/_strptime.py Outdated
seconds = int(z[5:7] or 0)
microseconds = int(z[8:] or 0)
gmtoff = (hours * 60 * 60) + (minutes * 60) + seconds
gmtoff = gmtoff + (microseconds / (10**6))

@abalkin abalkin Oct 24, 2017

Copy link
Copy Markdown
Member

There was a problem hiding this comment.

Choose a reason for hiding this comment

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

Let's not make gmtoff a float, but instead add gmtoff_fraction to the returned tuple. See how %f format is handled.

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
All reactions
@mariocj89
Handle gmtoff miliseconds via a separate variable
5cfeeee
@mariocj89

mariocj89 commented Oct 24, 2017

Copy link
Copy Markdown
Contributor Author

Let's not make gmtoff a float, but instead add gmtoff_fraction to the returned tuple. See how %f format is handled.

Great idea, I've added it at the end like the fraction, if you rather want it as part of the time tuple we can try that out as well.

Thanks @abalkin !
All reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
abalkin
abalkin approved these changes Oct 25, 2017
View reviewed changes
@bedevere-bot bedevere-bot added awaiting merge and removed awaiting change review labels Oct 25, 2017
@mariocj89
Enrich NEWS entry with latest changes
546b7b8
@mariocj89

mariocj89 commented Oct 25, 2017

Copy link
Copy Markdown
Contributor Author

@abalkin I've added some further information & corrected a typo in the NEWS entry.

Do you want me to add some info in Python3.7 what's new as well or it is not really worth it?
All reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
abalkin
abalkin requested changes Oct 25, 2017
View reviewed changes
Comment thread Doc/library/datetime.rst Outdated

.. versionchanged:: 3.7
When ``%z`` directive is provided to the :meth:`strptime` method, any valid
RFC-822/ISO 8601 standard utftime-offset can be parsed. For example, strings

@abalkin abalkin Oct 25, 2017

Copy link
Copy Markdown
Member

There was a problem hiding this comment.

Choose a reason for hiding this comment

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

utftime-offset? This should be UTC offset, right?

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
All reactions
Comment thread Misc/NEWS.d/next/Library/2017-10-17-20-08-19.bpo-31800.foOSCi.rst Outdated
@@ -0,0 +1,3 @@
Add support for parsing RFC-822/ISO 8601 UTC offsets. strptime '%z' can now

@abalkin abalkin Oct 25, 2017

Copy link
Copy Markdown
Member

There was a problem hiding this comment.

Choose a reason for hiding this comment

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

I don't think the RFC or the ISO standard allows seconds, let alone microseconds in the UTC offset. Please note that RFC 822 was obsoleted by RFC 2822 which in turn was obsoleted by 5322. I don't think this matters much for this PR because the RFCs specify the TZ format without a colon. I would remove references to standards and just describe what is implemented.

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
All reactions
@bedevere-bot

bedevere-bot commented Oct 25, 2017

Copy link
Copy Markdown

A Python core developer has requested some changes be made to your pull request before we can consider merging it. If you could please address their requests along with any other requests in other reviews from core developers that would be appreciated.

Once you have made the requested changes, please leave a comment on this pull request containing the phrase I have made the requested changes; please review again. I will then notify any core developers who have left a review that you're ready for them to take another look at this pull request.
All reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
@bedevere-bot bedevere-bot added awaiting changes and removed awaiting merge labels Oct 25, 2017
@mariocj89
Update docs on new %z behaviour
c20b57f
@mariocj89

mariocj89 commented Oct 25, 2017

Copy link
Copy Markdown
Contributor Author

I have made the requested changes; please review again
All reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
@bedevere-bot bedevere-bot added awaiting change review and removed awaiting changes labels Oct 25, 2017
@bedevere-bot

bedevere-bot commented Oct 25, 2017

Copy link
Copy Markdown

Thanks for making the requested changes!

@abalkin: please review the changes made to this pull request.
All reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
abalkin
abalkin approved these changes Oct 25, 2017
View reviewed changes
@bedevere-bot bedevere-bot added awaiting merge and removed awaiting change review labels Oct 25, 2017
@abalkin

abalkin commented Oct 26, 2017

Copy link
Copy Markdown
Member

Do you want me to add some info in Python3.7 what's new as well or it is not really worth it?

A NEWS entry is enough. The What's New editor will pick it from there.
All reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
@abalkin
abalkin merged commit 3231893 into python:master Oct 26, 2017
@bedevere-bot bedevere-bot removed the awaiting merge label Oct 26, 2017
@mariocj89 mariocj89 mentioned this pull request Dec 10, 2017
bpo-32267: Fix parsing tz offsets microseconds shorter than 6 #4781
Merged
@mariocj89
mariocj89 deleted the offset_colon branch January 6, 2018 17:32
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@abalkin abalkin abalkin approved these changes
@vstinner vstinner Awaiting requested review from vstinner

Uh oh!

There was an error while loading. Please reload this page.

Assignees

@abalkin abalkin

Labels

None yet

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

Uh oh!

There was an error while loading. Please reload this page.

4 participants

@mariocj89 @bedevere-bot @abalkin @the-knights-who-say-ni
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
You are visitor 01402503 — GeezerVison does not store the pages you look up.
Best viewed with Netscape Navigator 4.7 or Internet Explorer 6 at 800×600. No cookies required.