bpo-32697: Definition order of kwonly params is now preserved.#5391
Conversation
| :class:`Parameter` objects. Parameters appear in strict definition | ||
| order, including keyword-only parameters. | ||
|
|
||
| .. versionchanged:: 3.6 |
There was a problem hiding this comment.
I was struck by this, too. But after thinking about it, I think Larry's relying on 3.6 dicts being ordered. I'm relying on the same thing in my 3.6 dataclasses backport. There are no 3.6 implementations that don't have ordered dicts (since CPython is the only 3.6 implementation in the known universe, and if any others show up, they're likely to have ordered dicts, too). See https://mail.python.org/pipermail/python-dev/2017-December/151325.html for a brief analysis.
There was a problem hiding this comment.
Duh, you're right, I'll fix.
Eric: I'm not relying on 3.6 dicts being ordered. getfullargspec represents kwonly parameters in a list, inspect.Signature represents all parameters in an OrderedDict.
There was a problem hiding this comment.
Larry, btw, maybe we should switch inspect.Signature to use plain dicts (since they are ordered in 3.7)?
There was a problem hiding this comment.
IIUC the semantics of OrderedDict and order-preserving dicts are sliiiiiiightly different; at the very least OrderedDict has at least one extra method dict doesn't (OrderedDict.move_to_end()). On the one hand probably nobody would care, but on the other hand I'm not sure making the change would benefit anybody, either.
There was a problem hiding this comment.
IIUC the semantics of OrderedDict and order-preserving dicts are sliiiiiiightly different; at the very least OrderedDict has at least one extra method dict doesn't (OrderedDict.move_to_end()).
yep.
On the one hand probably nobody would care, but on the other hand I'm not sure making the change would benefit anybody, either.
dict is faster and more compact. And since, as you pointed out, OrderedDict.move_to_end is highly unlikely to be used in this context, I think it's an OK change for 3.7. But up to you.
There was a problem hiding this comment.
I don't think people using inspect.signature are very performance-sensitive. I think it's best to leave it alone. (Though I don't know why it's up to me... I was barely involved in inspect.signature!)
ericvsmith
left a comment
ericvsmith
left a comment
There was a problem hiding this comment.
Other than my one comment about only testing in lexigraphically sorted order, I think this looks good.
| Yields a whole bunch of functions with only keyword-only parameters, | ||
| where those parameters are always in lexigraphically sorted order. | ||
| """ | ||
| parameters = ['a', 'bar', 'c', 'delta', 'ephraim', 'magical', 'yoyo', 'z'] |
There was a problem hiding this comment.
I realize why you're generating parameters in sorted order here, but I think at least one test with non-sorted parameters might be a good idea, just to show the principle. Maybe also specifying them in reverse sorted order would be good enough?
There was a problem hiding this comment.
I'm not sure what you're proposing here. You want one test to fail, just to show that the detection mechanism is working? Or, you want me to hand-code one test where the parameters aren't sorted, just in case something in Python was sorting parameters?
There was a problem hiding this comment.
No, I want one test to pass that doesn't have the parameters in lexical order.
larryhastings
changed the title
|
@larryhastings: Please replace |
ericvsmith
left a comment
ericvsmith
left a comment
There was a problem hiding this comment.
Looks good to me. Now that I've reviewed, I can't figure out how to change my comment to "approved", but I do approve it (if that means anything!).
|
Never mind. It's already committed! Maybe that's why I can't approve. |
5 participants
Python now explicitly preserves the definition order of keyword-only parameters. It's always preserved their order, but this behavior was never guaranteed before; this behavior is now guaranteed and tested.
https://bugs.python.org/issue32697