PEP 692: Introduce proposed enhancements and expand Motivation section #2732
Conversation
|
All commit authors signed the Contributor License Agreement. |
Co-authored-by: CAM Gerlach <CAM.Gerlach@Gerlach.CAM>
pep-0692.rst
Outdated
| Moreover, one pattern that ``**kwargs`` are especially useful for is when a | ||
| function should accommodate optional keyword-only arguments that don't have | ||
| default values. A need for a pattern like that can arise when values that are |
There was a problem hiding this comment.
I find this a particularly weak argument -- optional keyword args without defaults seems an obscure edge case, and marker objects usually work just fine. Even if there are real-world examples of this need (as you show below), I don't think it's worth nearly 50 lines of text in the PEP.
There was a problem hiding this comment.
For what its worth, as a random datapoint from a random Python user, I don't think I've ever seen this pattern in real life—I'm sure it happens, but "especially useful" seems like it might be overstating it and this might be more helpful nearer the end of the section, particularly considering the utility of the other two (I see them absolutely everywhere).
There was a problem hiding this comment.
That makes sense, I deleted the detailed example and just linked the httpx issue. Also, moved the paragraphs around and changed the "especially useful" bit to "also convenient".
pep-0692.rst
Outdated
| In addition, ``**kwargs`` can be used to reduce the amount of code needed in | ||
| cases when there is a top-level method that is a part of a public API and it | ||
| calls a bunch of helper functions, all of which expect the same keyword | ||
| arguments. Unfortunately, if those helper functions were to use ``**kwargs``, | ||
| there is no way to properly type hint them if the keyword arguments they expect | ||
| are of different types. |
There was a problem hiding this comment.
This strikes me as the main use case -- the need to avoid repetition when one function calls another with an identical signature. Maybe you could elaborate more on how frequent this pattern is? I've encountered it enough to think it's quite often -- e.g. I recall writing many wrappers for open() that were a pain to write. I've also seen real bugs reported that were due to such a situation where the helper was updated to accept a new keyword and the wrapper wasn't.
(The other legit use case is adding type annotations to a legacy code base that uses **kwargs wrappers a lot -- I agree with what you wrote at the top of the Motivation section about exceeding the time budget for the migration.)
There was a problem hiding this comment.
Added the example you mentioned to the paragraph. Also, in the next paragraph I linked mypy issue 4441 and mentioned that it links to specific examples - on one hand feels sort of lazy but I think it is a nice way to show that there is indeed a real need for that proposal.
pep-0692.rst
Outdated
| 10000 | As described in the :ref:`Intended Usage <pep-692-intended-usage>` section, | |
| using ``**kwargs`` is not always the best tool for the job, but still it is a | ||
| widely used pattern. As a consequence, there has been a | ||
| `lot of discussion <mypyIssue4441_>`__ around supporting more precise | ||
| ``**kwargs`` typing and it became a feature that would be valuable for a large | ||
| part of the Python community. |
There was a problem hiding this comment.
This paragraph is kind of hanging here by itself. Maybe it needs to be made part of the Introduction instead?
There was a problem hiding this comment.
Moved it so it appears earlier in the Motivation section - without a detailed example of "facilitating optional keyword arguments that don't have default values" I think it is a nice transition from the previous two paragraphs.
pep-0692.rst
Outdated
| Moreover, one pattern that ``**kwargs`` are especially useful for is when a | ||
| function should accommodate optional keyword-only arguments that don't have | ||
| default values. A need for a pattern like that can arise when values that are |
There was a problem hiding this comment.
For what its worth, as a random datapoint from a random Python user, I don't think I've ever seen this pattern in real life—I'm sure it happens, but "especially useful" seems like it might be overstating it and this might be more helpful nearer the end of the section, particularly considering the utility of the other two (I see them absolutely everywhere).
Co-authored-by: Guido van Rossum <gvanrossum@gmail.com> Co-authored-by: CAM Gerlach <CAM.Gerlach@Gerlach.CAM>
|
Thank you for the pointers! Moved things around a bit, think it's better than it was, let me know what you think. |
pep-0692.rst
Outdated
| Moreover, ``**kwargs`` can be used to reduce the amount of code needed in | ||
| cases when there is a top-level function that is a part of a public API and it | ||
| calls a bunch of helper functions, all of which expect the same keyword | ||
| arguments. Unfortunately, if those helper functions were to use ``**kwargs``, | ||
| there is no way to properly type hint them if the keyword arguments they expect | ||
| are of different types. | ||
| are of different types. However, even if the keyword arguments are of the same | ||
| type, there is no way to check if the function is being called with keyword | ||
| names that it actually expects. All of this makes certain issues difficult to | ||
| spot. For example, a helper function could be updated to take account of a new | ||
| keyword argument, but the public function which uses the helper may not have it | ||
| in its signature. |
There was a problem hiding this comment.
I've got a feeling that there's an even shorter paragraph trying to come forward here. The key issue is that if we have e.g. xmlopen(file, mode) calling the builtin open(file, mode), and open() grows a new feature (say, bufsize=...), we might want to be warned that xmlopen() also needs to be updated to add that feature. Unfortunately, if the solution us that open() should be declared using (**kwds: **SomeTypeDict), I'm not convinced that this is altogether an improvement -- inside open() it's harder to access the arguments (kwargs["filename"] instead of just filename) and for the human reader the true signature is farther removed from the function definition.
There was a problem hiding this comment.
That makes sense, I agree that in the example that you're mentioning here there is no advantage to using **kwargs (btw, feels like it'd be quite hard to come up with an elegant solution to that). The situation I had in mind was that for example a helper which already uses **kwargs in its signature gets updated to deal with a new keyword (it uses kwagrs["new_keyword"] in its body), but a function that calls the helper doesn't take that into account - I guess the earlier paragraph would fit this example better. But anyway I've removed it altogether as it seems to be a well known use case for which static typing is helpful and it seems to me now that there is no point to highlight this one particular advantage.
pep-0692.rst
Outdated
| Another useful pattern that justifies using and typing ``**kwargs`` as proposed | ||
| is when the function's API should allow for optional keyword arguments that | ||
| don't have default values. | ||
|
|
There was a problem hiding this comment.
I know you just moved this paragraph -- but it's also added to the Motivation section. Why the duplication?
There was a problem hiding this comment.
That's a good point, I reworked the "Intended Usage" section so that the examples detailed in the "Motivation" section are not repeated - there is only a short summary of those to make the transition to "better tools for the job" paragraph smoother.
pep-0692.rst
Outdated
| ``TypedDict`` is the only type in the standard library that is expected to | ||
| implement ``__typing_kwargs_unpack__``, which should return ``Unpack[self]``. |
There was a problem hiding this comment.
This also feels like needless duplication.
There was a problem hiding this comment.
Kept this sentence here and removed it from the Abstract instead - in my opinion it fits this section better (there's more context around introducing a new dunder) and for abstracts I like to include as little detail as possible.
|
No, I think those are valid points. Thanks again for taking a look at this! |
Co-authored-by: CAM Gerlach <CAM.Gerlach@Gerlach.CAM>
Co-authored-by: Jelle Zijlstra <jelle.zijlstra@gmail.com>
|
Thanks Jelle! @gvanrossum also made changes around things you mentioned earlier, if there are no other issues I guess this should be ready to merge? |
|
Sorry this should have been merged already. Will merge now. |
Introduces proposed enhancements from the discuss thread and expands the "Motivation" section.