GeezerVisonThe World Wide Web, restored to 800 pixels of glory. | ||||||||||||||||||||||||||||||||||||||||||||||||||||
| Home|About|Gaming cabinet|Archives|How it works|Starter sites | ||||||||||||||||||||||||||||||||||||||||||||||||||||
GeezerVison is showing github.com — bpo-37645: add new function _PyObject_FunctionStr() by jdemeyer · Pull Request #14890 · python/cpython Conversation
|
|
I would prefer to make it private at the beginning: replace Py with _Py. |
Sorry, something went wrong.
There was an error while loading. Please reload this page.
Done. |
Sorry, something went wrong.
There was an error while loading. Please reload this page.
There was a problem hiding this comment.
The reason will be displayed to describe this comment to others. Learn more.
Sorry, something went wrong.
There was an error while loading. Please reload this page.
There was an error while loading. Please reload this page.
There was an error while loading. Please reload this page.
There was a problem hiding this comment.
The reason will be displayed to describe this comment to others. Learn more.
Why not using the qualified name for type(func)?
Would it be possible to also add the module, except for builtin functions?
Sorry, something went wrong.
There was an error while loading. Please reload this page.
There was a problem hiding this comment.
The reason will be displayed to describe this comment to others. Learn more.
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".
Sorry, something went wrong.
There was an error while loading. Please reload this page.
There was an error while loading. Please reload this page.
|
Before changing this PR, could you reply on my last comment on bpo-37645? It might be a better solution to just use |
Sorry, something went wrong.
There was an error while loading. Please reload this page.
There was an error while loading. Please reload this page.
There was an error while loading. Please reload this page.
There was an error while loading. Please reload this page.
| set_add = set.add | ||
|
|
||
| expected_errmsg = "descriptor 'add' of 'set' object needs an argument" | ||
| expected_errmsg = "set.add() needs an argument" |
There was a problem hiding this comment.
The reason will be displayed to describe this comment to others. Learn more.
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
Sorry, something went wrong.
There was an error while loading. Please reload this page.
There was a problem hiding this comment.
The reason will be displayed to describe this comment to others. Learn more.
Agree, this is confusing.
Sorry, something went wrong.
There was an error while loading. Please reload this page.
There was a problem hiding this comment.
The reason will be displayed to describe this comment to others. Learn more.
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.
Sorry, something went wrong.
There was an error while loading. Please reload this page.
| .. 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 |
There was a problem hiding this comment.
The reason will be displayed to describe this comment to others. Learn more.
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.
Sorry, something went wrong.
There was an error while loading. Please reload this page.
There was a problem hiding this comment.
The reason will be displayed to describe this comment to others. Learn more.
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__.
Sorry, something went wrong.
There was an error while loading. Please reload this page.
There was an error while loading. Please reload this page.
| set_add = set.add | ||
|
|
||
| expected_errmsg = "descriptor 'add' of 'set' object needs an argument" | ||
| expected_errmsg = "set.add() needs an argument" |
There was a problem hiding this comment.
The reason will be displayed to describe this comment to others. Learn more.
Agree, this is confusing.
Sorry, something went wrong.
There was an error while loading. Please reload this page.
There was an error while loading. Please reload this page.
|
I tried to address all comments. |
Sorry, something went wrong.
There was an error while loading. Please reload this page.
There was an error while loading. Please reload this page.
|
One more nitpick above. Otherwise, this does look like an improvement in the error messages – not only in custom extension callables! |
Sorry, something went wrong.
There was an error while loading. Please reload this page.
|
@jdemeyer: Status check is done, and it's a success ✅ . |
Sorry, something went wrong.
There was an error while loading. Please reload this page.
|
Sorry, I can't merge this PR. Reason: |
Sorry, something went wrong.
There was an error while loading. Please reload this page.
|
@jdemeyer: Status check is done, and it's a success ✅ . |
Sorry, something went wrong.
There was an error while loading. Please reload this page.
|
Sorry, I can't merge this PR. Reason: |
Sorry, something went wrong.
There was an error while loading. Please reload this page.
|
@jdemeyer: Status check is done, and it's a success ✅ . |
Sorry, something went wrong.
There was an error while loading. Please reload this page.
|
Sorry, I can't merge this PR. Reason: |
Sorry, something went wrong.
There was an error while loading. Please reload this page.
|
@zooba What's the best way to get Azure Pipelines to run on this PR? |
Sorry, something went wrong.
There was an error while loading. Please reload this page.
|
Thanks for going through with this. |
Sorry, something went wrong.
There was an error while loading. Please reload this page.
|
Shouldn't |
Sorry, something went wrong.
There was an error while loading. Please reload this page.
|
Good catch, thank you! |
Sorry, something went wrong.
There was an error while loading. Please reload this page.
The conflicting change in master is to use _PyErr_Format (with explicit thread state argument) instead of PyErr_Format
|
@jdemeyer: Status check is done, and it's a success ✅ . |
Sorry, something went wrong.
There was an error while loading. Please reload this page.
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
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
There was an error while loading. Please reload this page.
Successfully merging this pull request may close these issues.
There was an error while loading. Please reload this page.