-
-
Notifications
You must be signed in to change notification settings - Fork 2.8k
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
Overload docs #3159
Overload docs #3159
Conversation
The docs say `@override` doesn't work in user code, but it seems to work in mypy 0.470. The update may be waiting on #2603, but that PR does not seem to include doc updates, so feel free to put this patch in that PR.
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.
Thank you. I especially like the __getitem__
motivation, because it's a thing the user is quite likely to run into, unlike trying to redefine abs
.
My commentary is mostly picky wordsmithing stuff, and as such is a matter of judgement. Take that of it which seems to make the writing clearer, and leave the rest.
docs/source/function_overloading.rst
Outdated
my_abs = abs | ||
my_abs(-2) # 2 (int) | ||
my_abs(-1.5) # 1.5 (float) | ||
they still define a single runtime object. There is no multiple |
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.
s/multiple/automatic/
The user does implement dispatch in their implementation.
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.
Agreed, and also: this is single dispatch rather than multiple dispatch, since only one argument determines the implementation. But the sentence should apply to both single and multiple dispatch of course.
docs/source/function_overloading.rst
Outdated
my_abs(-2) # 2 (int) | ||
my_abs(-1.5) # 1.5 (float) | ||
they still define a single runtime object. There is no multiple | ||
dispatch happening, and you must manually handle the different types |
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.
"manually handle the different types in the implementation"
docs/source/function_overloading.rst
Outdated
else: | ||
assert False, "Unsupported argument %r" % (index,) | ||
|
||
But this is a little loose, as it implies that when you put in an |
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.
s/a little/too/
docs/source/function_overloading.rst
Outdated
pass # Don't put code here | ||
|
||
# All overloads and the implementation must be adjacent | ||
# in the source file, and overload order may matter. |
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.
Consider explaining how overload order matters -- without doing so it feels like a warning against using overloading, because stuff you don't understand might happen.
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.
See #1270 (comment) for some background.
docs/source/function_overloading.rst
Outdated
def __getitem__(self, index: slice) -> Sequence[T]: | ||
pass # Don't put code here | ||
|
||
# Actual implementation goes last, without @overload. |
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.
s/Actual/The/
docs/source/function_overloading.rst
Outdated
elif isinstance(index, slice): | ||
... # Return a sequence of Ts here | ||
else: | ||
assert False, "Unsupported argument %r" % (index,) |
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.
raise TypeError("Unsupported index type '%s'" % index.__class__.__name__)
or something like that is closer to what Python does in the builtin types.
docs/source/function_overloading.rst
Outdated
yet) using the ``@overload`` decorator. For example, we can define an | ||
``abs`` function that works for both ``int`` and ``float`` arguments: | ||
Sometimes the types in a function depend on each other in ways that | ||
can't be captured with a simple ``Union``. For example, the |
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.
s/simple//
docs/source/function_overloading.rst
Outdated
|
||
But this is a little loose, as it implies that when you put in an | ||
``int`` you might sometimes get out a single item or sometimes a | ||
sequence. To capture a constraint such as a return type that depends |
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 seems like your sentence structure here is a bit convoluted, and it might do to mention a type variable as another option here -- it's preferable when it's possible. Perhaps:
"The return type depends on the parameter type in a way that can't be expressed by a type variable. We can use overloading (link) to give the same function multiple type annotations (signatures) and accurately describe the function's behavior."
Or something -- wordsmithing is hard. My confidence that this version is better than yours is only about 70%.
OK, how about now? |
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 like it!
docs/source/function_overloading.rst
Outdated
... # Return a sequence of Ts here | ||
else: | ||
assert False, "Unsupported argument %r" % (index,) | ||
|
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.
Maybe just inherit from Generic[T]
? Otherwise, this code would fail with Signature of "__getitem__" incompatible with supertype "Sequence"
.
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.
Well, that's kind of the point (though I agree it's pretty implicit here).
docs/source/function_overloading.rst
Outdated
pass # Don't put code here | ||
|
||
# Actual implementation goes last, without @overload. | ||
# It may or may not have type hints; if it does, |
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.
See #3160 for my concerns with both untyped and typed implementation. Maybe worth warning in the docs that untyped definitions result in the assumption of Any
for each of the arguments (at least at the moment) -- that's if the body is even checked at all.
docs/source/function_overloading.rst
Outdated
yet) using the ``@overload`` decorator. For example, we can define an | ||
``abs`` function that works for both ``int`` and ``float`` arguments: | ||
Sometimes the types in a function depend on each other in ways that | ||
can't be captured with a simple ``Union``. For example, the |
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.
maybe "can't be captured with a Union
or generics
"?
docs/source/function_overloading.rst
Outdated
pass # Don't put code here | ||
|
||
# All overloads and the implementation must be adjacent | ||
# in the source file, and overload order may matter. |
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.
See #1270 (comment) for some background.
docs/source/function_overloading.rst
Outdated
my_abs(-2) # 2 (int) | ||
my_abs(-1.5) # 1.5 (float) | ||
they still define a single runtime object. There is no multiple | ||
dispatch happening, and you must manually handle the different types |
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.
Perhaps add that using @functools.singledispatch
with @overloaded
is not yet supported.
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.
Actually, @overload
is not limited to a single argument, and at the type system level you can totally express multi-dispatch with it.
docs/source/function_overloading.rst
Outdated
my_abs = abs | ||
my_abs(-2) # 2 (int) | ||
my_abs(-1.5) # 1.5 (float) | ||
they still define a single runtime object. There is no multiple |
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.
Agreed, and also: this is single dispatch rather than multiple dispatch, since only one argument determines the implementation. But the sentence should apply to both single and multiple dispatch of course.
Since I already landed this version, feel free to submit a PR with proposed improvements. (Though I will be on vacation through Monday.) |
This is based on PR #2792 by @jkleint. It documents the feature added in PR @2603 by @sixolet.
@sixolet, @ilevkivskyi, @pkch -- any comments on this version?