gh-64376: Convert the time module to the Argument Clinic by ZackerySpytz · Pull Request #14311 · python/cpython (original) (raw)
Conversation
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.Learn more about bidirectional Unicode characters
[ Show hidden characters]({{ revealButtonHref }})
Co-Authored-By: Nikolaus Rath nikolaus@rath.org
As mentioned on the BPO issue, it doesn't seem practical to use the Argument Clinic instead of the gettmarg() utility function.
The time_clock_gettime() function was not converted -- there's an ifdef involving the PyArg_ParseTuple() call on AIX.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
There are now (in current master) dedicated built-in functions to use for testing, which aren't very likely to gain Argument Clinic signatures. I suggest using _testcapi.meth_varargs, which returns (its module, tuple of arguments):
import _testcapi
meth = types.MethodType(_testcapi.meth_varargs, 32)
self.assertEqual(meth(), (_testcapi, (32,)))
There are now (in current master) dedicated built-in functions to use for testing, which aren't very likely to gain Argument Clinic signatures. I suggest using
_testcapi.meth_varargs, which returns (its module, tuple of arguments):
This PR shouldn't change test_gdb.py at all. The only reason that it was changed in this PR was to fix a test failure because test_gdb.py was using gmtime().
It no longer changes test_gdb. My comment was about Lib/unittest/test/testmock/testhelpers.py.
Sorry for the confusion.
| /*[clinic input] |
|---|
| time.gmtime |
| seconds: object = NULL |
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Maybe use a converter?
| seconds: object = NULL |
|---|
| seconds as when: object(converter="object_to_time_t", type="time_t") = None |
Or introduce a named converter?
Note also that you can use None as a default value for better signature.
| time.strptime |
|---|
| string: object |
| format: object = NULL |
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
| format: object = NULL |
|---|
| format: object(c_default="NULL") = "%a %b %d %H:%M:%S %Y" |
| { |
|---|
| PyObject *module, *func, *result; |
| PyObject *mod, *func, *result; |
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Why rename module?
Comment on lines +1371 to +1373
| Process time for profiling as nanoseconds: |
|---|
| sum of the kernel and user-space CPU time. |
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It is a single sentence. Either write it in one line or make separate sentences.
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.
bpo-20177: Convert the time module to the Argument Clinic bpo-64376: Convert the time module to the Argument Clinic
bpo-64376: Convert the time module to the Argument Clinic gh-64376: Convert the time module to the Argument Clinic
No response since 2020. In the three years since 2020, there has been a lot of changes to Modules/timemodule.c1. I suggest to close this PR; if someone wants to pick this up, it will probably be easier to start from scratch, rather than rebasing this PR.
Footnotes
- 569 insertions(+), 231 deletions(-) ↩