Skip to content
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

bpo-20177: Convert the time module to the Argument Clinic #14311

Open
wants to merge 2 commits into
base: main
Choose a base branch
from

Conversation

Copy link
Contributor

@ZackerySpytz ZackerySpytz commented Jun 22, 2019

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

@ZackerySpytz ZackerySpytz commented Jun 22, 2019

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.

Lib/test/test_gdb.py Outdated Show resolved Hide resolved
Copy link
Contributor

@jdemeyer jdemeyer left a comment

Needs #14330

@jdemeyer
Copy link

@jdemeyer jdemeyer commented Aug 5, 2019

Needs #14795 instead.

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

@encukou 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

@jdemeyer jdemeyer 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):

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

@encukou 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

@serhiy-storchaka serhiy-storchaka Jul 10, 2020

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

@serhiy-storchaka serhiy-storchaka Jul 10, 2020

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

@serhiy-storchaka serhiy-storchaka Jul 10, 2020

Choose a reason for hiding this comment

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

Why rename module?

Process time for profiling as nanoseconds:
sum of the kernel and user-space CPU time.
Copy link
Member

@serhiy-storchaka serhiy-storchaka Jul 10, 2020

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

@bedevere-bot bedevere-bot commented Jul 10, 2020

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

@taleinat taleinat commented Oct 3, 2020

Ping, @ZackerySpytz?

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.

None yet

7 participants