bpo-37645: add new function _PyObject_FunctionStr() by jdemeyer · Pull Request #14890 · python/cpython

jdemeyer · 2019-07-21T19:15:16Z

Additional note: the method_check_args function in Objects/descrobject.c is written in such a way that it applies to all kinds of descriptors. In particular, a future re-implementation of wrapper_descriptor could use that code.

CC @vstinner @encukou

https://bugs.python.org/issue37645

Automerge-Triggered-By: @encukou

vstinner · 2019-07-29T16:02:33Z

I would prefer to make it private at the beginning: replace Py with _Py.

jdemeyer · 2019-08-04T21:51:38Z

I would prefer to make it private at the beginning: replace Py with _Py.

Done.

vstinner

cc @serhiy-storchaka

vstinner · 2019-08-12T14:11:53Z

Why not using the qualified name for type(func)?
Would it be possible to also add the module, except for builtin functions?

Why not using the qualified name for type(func)?
I decided to use str(func) as fallback, which looks more useful than f"{type(func)} object".

jdemeyer · 2019-08-13T09:26:45Z

Before changing this PR, could you reply on my last comment on bpo-37645? It might be a better solution to just use str(func) in the error message and then changing some __str__ implementations.

encukou · 2019-09-11T15:25:54Z

 set_add = set.add

- expected_errmsg = "descriptor 'add' of 'set' object needs an argument"
+ expected_errmsg = "set.add() needs an argument"


I consider this a regression: the message is now too similar to calling the method on an instance, but missing an argument.
Traceback (most recent call last): File "<stdin>", line 1, in <module> TypeError: set.add() takes exactly one argument (0 given) >>> set.add() Traceback (most recent call last): File "<stdin>", line 1, in <module> TypeError: set.add() needs an argument

Agree, this is confusing.

I can change the error message to anything you like, except that it must contain the string set.add (the function name). So it could be
TypeError: unbound method set.add() needs an argument
or whatever (surely, this is better than anything mentioning descriptors).
Small rant: the bug is really this:
>>> set().add() Traceback (most recent call last): File "<stdin>", line 1, in <module> TypeError: add() takes exactly one argument (0 given)
For a Python method, it would correctly note that 2 arguments are required. This is difficult to fix in CPython since builtin_function_or_method doesn't really know whether it's a function or method. This was one of the things that the rejected PEP 580 would have addressed.

serhiy-storchaka · 2019-09-13T09:19:22Z

+.. c:function:: PyObject* _PyObject_FunctionStr(PyObject *func)
+
+ Return a user-friendly string representation of the function-like object
+ *func*. This returns ``func.__qualname__ + "()"`` if there is a


Returning __qualname__ without module is not particularly useful.
If you want to return a full qualified name (long but unambiguous), return __module__ + '.' + __qualname__. __module__ can be omitted if it is 'builtins'.
If you want to return a short name (it is enough in many times), return __name__.
if __qualname__ is not available, you can use __name__ instead.

if __qualname__ is not available, you can use __name__ instead.
This is mainly meant as replacement for PyEval_GetFuncName and all classes supported by that function implement __qualname__. So I see little reason for the additional complexity of supporting __name__.

serhiy-storchaka · 2019-09-13T09:21:25Z

 set_add = set.add

- expected_errmsg = "descriptor 'add' of 'set' object needs an argument"
+ expected_errmsg = "set.add() needs an argument"


Agree, this is confusing.

jdemeyer · 2019-09-13T22:07:33Z

I tried to address all comments.

encukou · 2019-10-08T12:09:45Z

One more nitpick above. Otherwise, this does look like an improvement in the error messages – not only in custom extension callables!

miss-islington · 2019-10-22T13:03:44Z

@jdemeyer: Status check is done, and it's a success ✅ .

miss-islington · 2019-10-22T13:03:46Z

Sorry, I can't merge this PR. Reason: Required status check "Azure Pipelines PR" is expected..

miss-islington · 2019-10-22T13:20:05Z

@jdemeyer: Status check is done, and it's a success ✅ .

miss-islington · 2019-10-22T13:20:06Z

Sorry, I can't merge this PR. Reason: Required status check "Azure Pipelines PR" is expected..

miss-islington · 2019-10-22T13:30:06Z

@jdemeyer: Status check is done, and it's a success ✅ .

miss-islington · 2019-10-22T13:30:08Z

Sorry, I can't merge this PR. Reason: Required status check "Azure Pipelines PR" is expected..

encukou · 2019-10-22T13:55:49Z

@zooba What's the best way to get Azure Pipelines to run on this PR?
I asked the same at #16497 (comment) and things got moving when I did. Was that you pushing a button somewhere?

jdemeyer · 2019-10-22T18:38:32Z

Thanks for going through with this.

rlamy · 2019-10-31T17:30:43Z

Shouldn't format_kwargs_error be changed as well, to stay consistent with check_args_iterable?

encukou · 2019-11-05T14:53:57Z

Good catch, thank you!

The conflicting change in master is to use _PyErr_Format (with explicit thread state argument) instead of PyErr_Format

miss-islington · 2019-11-05T15:48:01Z

@jdemeyer: Status check is done, and it's a success ✅ .

@vstinner

Additional note: the `method_check_args` function in `Objects/descrobject.c` is written in such a way that it applies to all kinds of descriptors. In particular, a future re-implementation of `wrapper_descriptor` could use that code. CC @vstinner @encukou https://bugs.python.org/issue37645 Automerge-Triggered-By: @encukou

@vstinner

Additional note: the `method_check_args` function in `Objects/descrobject.c` is written in such a way that it applies to all kinds of descriptors. In particular, a future re-implementation of `wrapper_descriptor` could use that code. CC @vstinner @encukou https://bugs.python.org/issue37645 Automerge-Triggered-By: @encukou

the-knights-who-say-ni added the CLA signed label Jul 21, 2019

bedevere-bot added the awaiting review label Jul 21, 2019

ZackerySpytz reviewed Jul 29, 2019
View reviewed changes

Comment threadObjects/object.c Outdated

jdemeyer changed the title ~~bpo-37645: add new function PyObject_FunctionStr()~~bpo-37645: add new function _PyObject_FunctionStr()Aug 4, 2019

vstinner reviewed Aug 12, 2019
View reviewed changes

jdemeyerand others added 4 commits August 18, 2019 18:18

bpo-37645: add new function PyObject_FunctionStr()
2a55a9f

bpo-37645: clear exception in check_args_iterable()
733158d

bpo-37645: rename PyObject_FunctionStr() -> _PyObject_FunctionStr()
4c0c877

bpo-37645: use str(func) as fallback in _PyObject_FunctionStr()
2e1b44e

jdemeyer force-pushed the funcstr branch from ab65a48 to 2e1b44eCompare August 18, 2019 18:16

encukou reviewed Sep 11, 2019
View reviewed changes

serhiy-storchaka reviewed Sep 13, 2019
View reviewed changes

bpo-37645: review fixes
fa20556

encukou reviewed Oct 8, 2019
View reviewed changes

Comment threadObjects/object.c

encukou added 2 commits October 22, 2019 14:14

Handle error in PyObject_RichCompareBool
b2e56da

Add author to NEWS
21aee80

encukou approved these changes Oct 22, 2019
View reviewed changes

bedevere-bot added awaiting merge and removed awaiting review labels Oct 22, 2019

encukou added the 🤖 automerge label Oct 22, 2019

Change format_kwargs_error to use _PyObject_FunctionStr as well
402ec96

encukou force-pushed the funcstr branch from edef14c to 402ec96Compare November 5, 2019 15:00

Merge branch 'master' to resolve PyErr_Format/_PyErr_Format conflict
0307ce9
The conflicting change in master is to use _PyErr_Format (with explicit thread state argument) instead of PyErr_Format

miss-islington merged commit bf17d41 into python:masterNov 5, 2019

bedevere-bot removed the awaiting merge label Nov 5, 2019

jdemeyer mentioned this pull request Oct 16, 2022
Replace PyEval_GetFuncName/PyEval_GetFuncDesc #81826
Open

Uh oh!

Uh oh!

Conversation

jdemeyer commented Jul 21, 2019• edited by miss-islington Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Uh oh!

vstinner commented Jul 29, 2019

Uh oh!

jdemeyer commented Aug 4, 2019

Uh oh!

vstinner left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

jdemeyer commented Aug 13, 2019• edited by bedevere-bot Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

jdemeyer commented Sep 13, 2019

Uh oh!

Uh oh!

encukou commented Oct 8, 2019

Uh oh!

miss-islington commented Oct 22, 2019

Uh oh!

miss-islington commented Oct 22, 2019

Uh oh!

miss-islington commented Oct 22, 2019

Uh oh!

miss-islington commented Oct 22, 2019

Uh oh!

miss-islington commented Oct 22, 2019

Uh oh!

miss-islington commented Oct 22, 2019

Uh oh!

encukou commented Oct 22, 2019

Uh oh!

jdemeyer commented Oct 22, 2019

Uh oh!

rlamy commented Oct 31, 2019

Uh oh!

encukou commented Nov 5, 2019

Uh oh!

miss-islington commented Nov 5, 2019

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

9 participants

jdemeyer commented Jul 21, 2019•
edited by miss-islington
Loading

jdemeyer commented Aug 13, 2019•
edited by bedevere-bot
Loading