Skip to content

WIP: bpo-1100942: Add datetime.time.strptime and datetime.date.strptime #5578

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.

Sign up for GitHub

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

Closed
matrixise wants to merge 16 commits into from
Closed

WIP: bpo-1100942: Add datetime.time.strptime and datetime.date.strptime #5578

Show file tree
Hide file tree
Changes from 1 commit
Commits
Show all changes
16 commits
Select commit Hold shift + click to select a range
ad5ffab
bpo-1100942: Add datetime.time.strptime and datetime.date.strptime
matrixise Feb 7, 2018
50606eb
Follow the PEP7 for the C Code
matrixise May 15, 2018
ab5030f
Remove unused formats
matrixise May 15, 2018
07fb326
Use subtest for the tests
matrixise Oct 17, 2018
010ac8b
Fix the signatures
matrixise Oct 25, 2018
5b58fc1
Update with the recommendation of @pganssle
matrixise Oct 25, 2018
8353a65
Update
matrixise Nov 4, 2018
cff74a9
Add use case where we generate an exception
matrixise Nov 11, 2018
f92a799
Add the Py_UNUSED(ignore) for datetime_gettime & datetime_getdate
matrixise Nov 11, 2018
4c4a7cc
Add a link to the strtime() and strptime() behaviour
matrixise Mar 6, 2019
b006172
Update the blurb entry and add an entry in the Doc/whatsnew/3.8.rst
matrixise Mar 6, 2019
ff7487f
Fix doc
matrixise Mar 6, 2019
a048f0e
Convert the _check_invalid_datetime_spec to the C-API
matrixise Mar 18, 2019
9ac0af4
Support tuple and list for the specs
matrixise Mar 18, 2019
a952bb7
_datetimemodule._check_invalid_datetime_specs only accepts tuple and not
matrixise Mar 24, 2019
412aea2
Fix the code with the recommendation of Martin Panter
matrixise Mar 24, 2019
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
Prev Previous commit
Next Next commit
Update with the recommendation of @pganssle
  • Loading branch information
@matrixise
matrixise committed Mar 22, 2019
commit 5b58fc1316e537eee5985dc0af60bce3d268a334
15 changes: 5 additions & 10 deletions Lib/_strptime.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -557,8 +557,8 @@ def _strptime(data_string, format="%a %b %d %H:%M:%S %Y"):
hour, minute, second,
weekday, julian, tz, tzname, gmtoff), fraction, gmtoff_fraction

date_specs = ('%a', '%A', '%b', '%B', '%c', '%d', '%j', '%m', '%U',
'%w', '%W', '%x', '%y', '%Y', '%G', '%u', '%V',)
date_specs = ('%a', '%A', '%b', '%B', '%c', '%d', '%j', '%m', '%U', '%G',
'%u', '%V', '%w', '%W', '%x', '%y', '%Y', '%G', '%u', '%V',)
time_specs = ('%H', '%I', '%M', '%S', '%f',)

def _strptime_time(data_string, format="%a %b %d %H:%M:%S %Y"):
Expand All @@ -585,29 +585,24 @@ def _strptime_datetime(cls, data_string, format="%a %b %d %H:%M:%S %Y"):

def _strptime_datetime_date(data_string, format):
"""Return a date based on the input string and the format string."""
if not format:
raise ValueError("Date format is not valid.")
msg = "'{!s}' {} not valid in date format specification."
if _check_invalid_datetime_specs(format, time_specs, msg):

@vadmium vadmium Mar 23, 2019

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Why not move the msg string into the check function, and just pass the bit that varies ('date' or 'time') as an argument? It would make the code easier to read.

Looks like the check function either raises an exception or returns True. It would be clearer to not use an if statement here.

@matrixise matrixise Mar 24, 2019

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

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

at the beginning, this PR was a submitted patch by other contributors, I just wanted to convert it to a PR. and now, I try to fix all the issues.

@matrixise matrixise Mar 24, 2019

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

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

but for this point, the first step was the conversion of _check_invalid_datetime_specs to C, and after start to improve the code with the other recommendations. (from @vstinner and @pganssle)

_date = _strptime_datetime(datetime_datetime, data_string, format)
return _date.date()

def _strptime_datetime_time(data_string, format):
"""Return a time based on the input string and the format string."""
if not format:
raise ValueError("Date format is not valid.")
msg = "'{!s}' {} not valid in time format specification."
if _check_invalid_datetime_specs(format, date_specs, msg):
_time = _strptime_datetime(datetime_datetime, data_string, format)
return _time.time()

def _check_invalid_datetime_specs(fmt, specs, msg):
def _check_invalid_datetime_specs(fmt, blacklist_specs, msg):
found_invalid_specs = []
for spec in specs:
for spec in blacklist_specs:
if spec in fmt:
found_invalid_specs.append(spec)
if found_invalid_specs:
suffix = "are" if len(found_invalid_specs) > 1 else "is"
raise ValueError(msg.format(", ".join(found_invalid_specs),
suffix))
raise ValueError(msg.format(", ".join(found_invalid_specs), suffix))
return True
12 changes: 7 additions & 5 deletions Lib/test/datetimetester.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -3129,12 +3129,14 @@ def test_strftime(self):
pass

def test_strptime_invalid(self):
tests = [('2004-12-01 13:02:47.197',
'%Y-%m-%d %H:%M:%S.%f'),
('2004-12-01', '%Y-%m-%d'),]
tests = [
('2004-12-01 13:02:47.197', '%Y-%m-%d %H:%M:%S.%f'),
('2004-12-01', '%Y-%m-%d'),
]
for date_string, date_format in tests:

@pganssle pganssle May 15, 2018

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

I think it would be good to use with self.subTest for these parametrized tests.

Also, per the other comment I guess you need to add something like ('12:30:15', '')` to get full coverage.

@pganssle pganssle Nov 4, 2018

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

I think you need two more test cases:

    ('1900-01-01 12:30', '%Y-%m-%d %H:%M'),
    ('12:30:15', ''),

date.strptime has similarly missing tests.

@matrixise matrixise Nov 11, 2018

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

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

the test with ('1900-01-01 12:30', '%Y-%m-%d %H:%M') does not raise an exception but returns datetime.time(12, 30)

For the other test, yep, there is an issue.

@pganssle pganssle Nov 11, 2018

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

The fact that it doesn't raise an exception is an issue. I'm a bit surprised that it doesn't raise an exception on pure Python, that's a bug, because I'm pretty sure that:

datetime.time.strptime("1901-01-01 12:30", "%Y-%m-%d %H:%M") does raise an exception.

with self.assertRaises(ValueError):
time.strptime(date_string, date_format)
with self.subTest(date_string=date_string, date_format=date_format):
with self.assertRaises(ValueError):
time.strptime(date_string, date_format)

def test_strptime_valid(self):
string = '13:02:47.197'
Expand Down