fix: Reject keyword argument None with .none(false)
#2611
Conversation
potpath
changed the title
None with .none(false)
…ributes.py::test_accepts_none
|
Thanks for reporting, @potpath! Weird that this was never encountered before. I've pushed a fix to your branch that hopefully fixes this! :-) |
| @@ -628,26 +628,30 @@ class cpp_function : public function { | |||
| bool copied_kwargs = false; | |||
|
|
|||
| for (; args_copied < num_args; ++args_copied) { | |||
| const auto &arg = func.args[args_copied]; | |||
| const auto &arg_rec = func.args[args_copied]; | |||
There was a problem hiding this comment.
This rename is, strictly speaking, unrelated, but to increase conformity with the naming in step // 1. above, and to more clearly indicate the type, I renamed it.
YannickJadoul
changed the title
None with .none(false)None with .none(false)
tests/test_methods_and_attributes.py
Outdated
| @@ -404,6 +404,19 @@ def test_accepts_none(msg): | |||
| assert m.ok_none4(None) == -1 | |||
| assert m.ok_none5(None) == -1 | |||
|
|
|||
| with pytest.raises(TypeError) as excinfo: | |||
| m.no_none_kw(None) | |||
There was a problem hiding this comment.
no_none_kw seems to suggest that one keyword arguments with None as the value are forbidden. At least that's how I'm reading it.
There was a problem hiding this comment.
Also after the others, no_none{1..5} and ok_none{1..5}?
Any suggestions? no_none_as_kw?
|
Let's add a change log entry too, unless this worked in 2.5. Might also grep and make sure this didn't have some other use or attempt to use elsewhere, and also make sure it's mentioned in the docs. |
Right, I overlooked that by building further on this existing PR.
Where would it? Tests pass, so I assume this wasn't tested or considered before? |
Fine to log under "Smaller or developer focused features and fixes:"? |
Good point, but I don't think so; I think it's ready. To me, this is almost purely a bug fix (since I don't think we mention in the docs that this only works for positional arguments?). |
|
I think it's fine with me. |
|
Thank you everyone for this prompt fix! |
Description
Demo a failing test case where passing keyword as
Nonetopy::arg("a").none(false)is still accepted.Suggested changelog entry:
N/A.
This PR is to show a failing test case that can be used as a starting point.