-
-
Notifications
You must be signed in to change notification settings - Fork 30.3k
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-37206: Unrepresentable default values no longer represented as None. #13933
bpo-37206: Unrepresentable default values no longer represented as None. #13933
Conversation
610914c
to
4d65caa
Compare
Lib/test/clinic.test
Outdated
@@ -92,7 +92,7 @@ test_objects_converter | |||
[clinic start generated code]*/ | |||
|
|||
PyDoc_STRVAR(test_objects_converter__doc__, | |||
"test_objects_converter($module, a, b=None, /)\n" | |||
"test_objects_converter($module, a, b=<unspecified>, /)\n" |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It would be considerable that <unspecified>
is obvious.
PEP 570 enabled us to make text signature and document consistent.
In document, we use [, b]
for optional argument without public default value.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I would prefer [, b]
if it be what the user see. But the average user will not see this, because the inspect module fails to parse both forms and the user will see just (...)
in the help output. The purpose of this change to prevent displaying a wrong result (b=None
when None is not acceptable). I do not see a sense of spending additional effort if the result is the same. I will make the representation using brances once I have found how to make it visible to the user.
I'm not going to get into whether this PR is appropriate for 3.8 but, assuming it were, I would be very concerned about making a change of this size and complexity in 3.7 at this stage in its lifecycle. |
Tools/clinic/clinic.py
Outdated
@@ -4400,7 +4399,7 @@ def bad_node(self, node): | |||
# mild hack: explicitly support NULL as a default value | |||
if isinstance(expr, ast.Name) and expr.id == 'NULL': | |||
value = NULL | |||
py_default = 'None' | |||
py_default = '<unspecified>' |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'd also caution against the use of words like "unspecified" or "undefined" because that can imply to a reader that the behavior is arbitrary and up to the Python implementation when not supplied. We do define these behaviors, they just require reading further documentation.
<see docs>
might be better? the brackets make it clear that it isn't an actual Python value no matter what text goes in there.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It does not matter what invalid syntax is used here. The end user will never see it. This is the simplest way to make the insect module not reporting the wrong result. The effect is the same as there is not any signature (as was before using Argument Clinic).
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
What do you think about <unrepresentable>
or <nonrepresentable>
?
(This will not be seen by an average Python user, it is only for core developers)
@@ -3204,6 +3201,8 @@ def converter_init(self, *, accept={str}, encoding=None, zeroes=False): | |||
# sorry, clinic can't support preallocated buffers | |||
# for es# and et# | |||
self.c_default = "NULL" | |||
if NoneType in accept and self.c_default == "Py_None": |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
is this really okay? we hold the GIL and the code you've modified at a quick first glance isn't using the value when it is == Py_None, but we never Py_INCREF'd the Py_None and there is nothing preventing us from treating it as if we had.
It also potentially adds situations where None is accepted as a parameter where it wasn't before? I'd need to go audit all of the APIs this change is trying to update to see if each of them intends to allow None as a value for a review of that. In that case if someone did pass in None from Python code, it should've been Py_INCREF'd - who's doing the DECREF before clobbering it?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This is the str
converter. The target's type is const char *
. Py_None is invalid value here. None is converted to NULL. For example see _io.open
and various encode/decode methods.
When you're done making the requested changes, leave the comment: And if you don't make the requested changes, you will be poked with soft cushions! |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Currenty it is just the simlest way to make the insect module failing to parse the signature instead of returning the false result. The end user will see the same resilt as if there is no any signature.
Lib/test/clinic.test
Outdated
@@ -92,7 +92,7 @@ test_objects_converter | |||
[clinic start generated code]*/ | |||
|
|||
PyDoc_STRVAR(test_objects_converter__doc__, | |||
"test_objects_converter($module, a, b=None, /)\n" | |||
"test_objects_converter($module, a, b=<unspecified>, /)\n" |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I would prefer [, b]
if it be what the user see. But the average user will not see this, because the inspect module fails to parse both forms and the user will see just (...)
in the help output. The purpose of this change to prevent displaying a wrong result (b=None
when None is not acceptable). I do not see a sense of spending additional effort if the result is the same. I will make the representation using brances once I have found how to make it visible to the user.
@@ -3204,6 +3201,8 @@ def converter_init(self, *, accept={str}, encoding=None, zeroes=False): | |||
# sorry, clinic can't support preallocated buffers | |||
# for es# and et# | |||
self.c_default = "NULL" | |||
if NoneType in accept and self.c_default == "Py_None": |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This is the str
converter. The target's type is const char *
. Py_None is invalid value here. None is converted to NULL. For example see _io.open
and various encode/decode methods.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Sorry for being terse. Posted from a phone.
Tools/clinic/clinic.py
Outdated
@@ -4400,7 +4399,7 @@ def bad_node(self, node): | |||
# mild hack: explicitly support NULL as a default value | |||
if isinstance(expr, ast.Name) and expr.id == 'NULL': | |||
value = NULL | |||
py_default = 'None' | |||
py_default = '<unspecified>' |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It does not matter what invalid syntax is used here. The end user will never see it. This is the simplest way to make the insect module not reporting the wrong result. The effect is the same as there is not any signature (as was before using Argument Clinic).
Thanks @serhiy-storchaka for the PR 🌮🎉.. I'm working now to backport this PR to: 3.8. |
Sorry, @serhiy-storchaka, I could not cleanly backport this to |
… as None. (pythonGH-13933) In ArgumentClinic, value "NULL" should now be used only for unrepresentable default values (like in the optional third parameter of getattr). "None" should be used if None is accepted as argument and passing None has the same effect as not passing the argument at all.. (cherry picked from commit 279f446) Co-authored-by: Serhiy Storchaka <storchaka@gmail.com>
GH-16141 is a backport of this pull request to the 3.8 branch. |
|
… as None. (GH-13933) (GH-16141) In ArgumentClinic, value "NULL" should now be used only for unrepresentable default values (like in the optional third parameter of getattr). "None" should be used if None is accepted as argument and passing None has the same effect as not passing the argument at all. (cherry picked from commit 279f446) Co-authored-by: Serhiy Storchaka <storchaka@gmail.com>
https://bugs.python.org/issue37206