Skip to content

gh-64376: Convert the time module to the Argument Clinic #14311

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.

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

Conversation

ZackerySpytz
Copy link
Contributor

@ZackerySpytz ZackerySpytz commented Jun 22, 2019

Co-Authored-By: Nikolaus Rath <nikolaus@rath.org>
@ZackerySpytz
Copy link
Contributor Author

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.

Copy link
Contributor

@jdemeyer jdemeyer left a comment

Choose a reason for hiding this comment

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

Needs #14330

@jdemeyer
Copy link
Contributor

jdemeyer commented Aug 5, 2019

Needs #14795 instead.

Since pythonGH-15776, the test no longer uses time_gmtime
@encukou
Copy link
Member

encukou commented Sep 10, 2019

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,)))

@jdemeyer
Copy link
Contributor

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().

@encukou
Copy link
Member

encukou commented Sep 10, 2019

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
Copy link
Member

Choose a reason for hiding this comment

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

Maybe use a converter?

Suggested change
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
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
format: object = NULL
format: object(c_default="NULL") = "%a %b %d %H:%M:%S %Y"

{
PyObject *module, *func, *result;
PyObject *mod, *func, *result;
Copy link
Member

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.
Copy link
Member

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.

@bedevere-bot
Copy link

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.

@taleinat
Copy link
Contributor

taleinat commented Oct 3, 2020

Ping, @ZackerySpytz?

@erlend-aasland erlend-aasland added the pending The issue will be closed if no feedback is provided label May 2, 2023
@erlend-aasland erlend-aasland changed the title bpo-20177: Convert the time module to the Argument Clinic bpo-64376: Convert the time module to the Argument Clinic May 2, 2023
@erlend-aasland erlend-aasland changed the title bpo-64376: Convert the time module to the Argument Clinic gh-64376: Convert the time module to the Argument Clinic May 2, 2023
@erlend-aasland
Copy link
Contributor

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

  1. 569 insertions(+), 231 deletions(-)

@erlend-aasland erlend-aasland removed the pending The issue will be closed if no feedback is provided label May 9, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

9 participants