We should probably consistently use "UTC offset", though I suppose it doesn't matter much since it's not a public-facing docstring.

@pganssle I am not really confident with the offsets and the datetime. Do you think we could keep it like that and propose an other bpo ?

I just meant use UTC instead of GMT everywhere.

Yeah, just replace "GMT" with "UTC". datetime.tzinfo has a utcoffset() method, so "UTC" is preferred in datetime.

(And there are some subtle differences between GMT and UTC that I forgot.)

I think this is missing%G,%u and%V, the ISO 8601 week calendar directives.

I don't see any references to%T,%R,%i or%s in the docs or elsewhere in the code. What do these represent?

If I'm understanding this correctly, this function seems to have the inverted sense of what I would expect. It seems that it checks iffmt is avalid spec.

From the names of the variables and functions, I was thinking that this would be a whitelist not a blacklist. Does it make sense to switch to a whitelist approach? If not, can we maybe changespecs to beblacklist_specs or something?

I don't have strong opinions about this, but my intuition is that these should beset orfrozenset rather thantuple. Are these tuples for performance reasons (I am not sure I know when exactly it's faster to use a set for "lookup membership" rather than a tuple or list).

This line and its equivalent in_time are not being hit. If I understand correctly this branch is only hit ifformat is empty?

useless, thanks

I think it would be good to usewith 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.

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.

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

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

The fact that it doesn't raise an exceptionis 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.

If I'm interpretingPEP 7 correctly, theseif statements need curly braces. The relevant section isCode layout.

Here and below, you havenonzero instead ofnon-zero. If we keep this wording, the hyphen needs to be added.

However, this PR does not parse to atimetuple or something and check if certain components were zero, it checks to see if theformat string containstime components, and fails in that case (Edit: I was looking at just the pure python implementation - I now realize that this is precisely what the C implementation is doing, but I think it's the wrong thing to do anyway). The way the docs are currently worded, you would expect this to work:

fromdatetimeimportdatedate.strptime("2018-01-01 00:00","%Y-%m-%d %H:%M")

But it will fail (rightly so, I think).

I believe you can change the last part of the last sentence as such:

-     parsed by `time.strptime`, or if it returns a value where the time part is-     nonzero.+     parsed by `time.strptime`, or if time components are present in the format string.

Also, I think it needs to be

:meth:`time.strptime`

One other note on the documentation, thedatetime.strptime documentation links to

:ref:`strftime-strptime-behavior`.

I think these should too.

I don't know why thisPyObject *Py_UNUSED(ignored) is here, so perChesterton's Fence, I'm not comfortable removing it. Any insight as to why it is here and what the consequences will be in removing it?

Possibly it will be a breaking change in the C ABI?

According togit blame,this is to silence a warning ingcc8. Is this still relevant@siddhesh@serhiy-storchaka?

Edit: Looking closer, the merged PR is from April, so I'm guessing it is, but now I'm wondering if this will break the C ABI theother way.

Yes,datetime_gettime should have two arguments, the second is ignored.

Hm, if I understand correctly, this is inconsistent with the pure Python version. The pure Python version checks if there are any time components in theformat string, whereas the C version parses to a datetime and then checks to see if there are anytime components.

I think checking the format string is the better way to do this, as I mention in another comment, this approach seems to indicate thatdate.strptime("2018-01-01 00:00", "%Y-%m-%d %H:%M") wouldsucceed, which is not the right thing to do. I will comment on the test suite to add a test for this.

ok, I will check the Python implementation, but in this case, we could migrate the Python implementation to the C layer?

Yeah, port the Python implementation to the C layer.

lol ;-) I am not an expert with the C-API, but I could try, it's a good exercise for my comprehension of the C-API of Python

Yeah, you have to reimplement _check_invalid_datetime_specs() in C.

Why does it have to be implemented in C? There are already calls out to the _strptime Python-language module here.

not sure, but because there is an other test in C for the date, and maybe we could put the verification process in only one function.@vstinner do you confirm?

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.

This needs at least two more test cases:

    ('2018-01-01 00:00','%Y-%m-%d %H:%M'),    ('2018-01-01',''),

Both should fail.

Perhaps clarify “time.strptime” does not refer to thestrptime function in thetime module. The reader may not see themeth RST code, HTML links, etc.

“Bytime.strptime” is redundant, unless you mean to refer to thestrptimefunction in thetime module.

What does “the date part is nonzero” mean? I would expect thetime class to work without specifying a date, zero or otherwise.

ValueErroris raised . . .

And microseconds should beor microseconds, unless you must combine all four codes to get the error.

But wouldn’t these details be better placed directly under thedate andtime classes, not in this common section about format codes in general?

I have fixed the grammar issues, but for the details I don't know.

Need to clarifytime.strptime refers to thetime module, not your newstrptime method.

👍

Why`time` here, but`.time` below (with a dot)?

👍 and there was an other erorr with*date_string*

I expect you lose the reference todate andtime, so all you are saying is you added two functions with the same name repeated.

@vadmium yep, here, we just add the class methodsdatetime.date.strptime anddatetime.time.strptime

Why would it be a list? This seems like unused code.

the first version was only for the tuples. After, I wanted to accept the list for my tests. but I am not against to only accept the tuples.

Why does it have to be implemented in C? There are already calls out to the _strptime Python-language module here.

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

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

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.

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)