-
Notifications
You must be signed in to change notification settings - Fork 0
Conversation
# Signature.bind_partial(). | ||
continue | ||
new_arguments.append((name, val)) | ||
bound_arguments.arguments = OrderedDict(new_arguments) |
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.
Why convert a list of pairs to an OrderedDict
? Why not just make an empty OrderedDict
and populate 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.
I copied the code from python3.5 sources, didn't want to change anything to official code.
bound_arguments.arguments = OrderedDict(new_arguments) | ||
else: | ||
def _apply_defaults(bound_arguments): | ||
return bound_arguments.apply_defaults() |
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.
bound_arguments.apply_defaults()
returns None
, but it's still weird that this version "returns" something while the backward-compatibility version doesn't.
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'm like a decorator, don't care about implementation detail, I want to be future-compatible.
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.
Future-compatibility-wise, if in the future apply_default will return something, someone might use the value returned from _apply_defaults believing it's backward compatible
assert d[AUTO] == AUTO | ||
assert d[MAX] == MAX | ||
assert d['<MAX>'] is MAX | ||
assert 'AUTO' not in d |
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.
Wait what? Isn't this a bug?
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'll revisit this at some point...
(already merged) |
No description provided.