How important is the `@_dispatchable` decorator in networkx?
Can this decorator be removed?
One obvious advantage for having it is to mirror runtime. It has some disadvantages though:
- I can't for the life of me find a way to use it on overloaded functions. Indeed looking at the stubs, overloaded functions are stripped of this decorator
- VS Code with Pylance does not show the documentation of the function on hover only when the decorator is used
I find the overload case very important that it outweighs the benefits of matching runtime. I also think the editor hover issue compelling on its own but one could argue that this should be discussed with the Pylance team instead.
I wonder if we could just get rid of the decorator altogether. Am I missing other reasons for having the decorator?
Without knowledge of networkx: For typeshed decorators are only relevant of they either have special meaning to type checkers (like @classmethod or @deprecated) or if they manipulate the signature somehow. Although in the latter case we might just use the modified signature instead. In some cases, keeping it as documentation might be worth it as well.
I don't claim to understand the documentation of the @_dispatchable decorator, but to me it sounds like it's just an implementation detail. If it causes practical problems with annotations, I'd suggest we should just remove the decorators.
@Avasam looking at networkx change history I saw that you are the most involved maintainer in these stubs. I'd like to hear your opinion on removing the @_dispatchable decorator.
For now, I've been leaving todo comments in places where the return type of a dispatchable function depends on an overload.
Sorry for the late answer !
@Avasam looking at networkx change history I saw that you are the most involved maintainer in these stubs.
Mostly because I reviewed + followed-up on @rhelmot 's initial PR and removed the networkx stubs from https://github.com/microsoft/python-type-stubs/pull/369 ^^
One obvious advantage for having it is to mirror runtime.
And to be more specific: it makes it easier and less surprising to stub. Especially if you don't know by heart what @_dispatchable does to the signature.
However, one of the more recent networkx updates added a lot more @_dispatchable usages, which made
I can't for the life of me find a way to use it on overloaded function
a lot more noticeable.
Am I missing other reasons for having the decorator?
I'd say it massively reduces the amount of manual overloads we need to do in the stubs.
See
That's 781 locations where you'd have to update a single @_dispatchable for 2 overloads to be accurate. Or we indiscriminately add **backend_kwargs even for when backend=None and loose on type safety there, but avoids using any decorator. I guess this should be caught by stubtest ?
Indeed overloading a function decorated with a callable who's __call__ method is itself overloaded is currently not supported by mypy or pyright.
To quote @erictraut : https://github.com/microsoft/pyright/issues/7167#issuecomment-1918097663
I think your use of
overloadhere is invalid, so I don't consider this a bug.An
@overloaddecorator should be applied to a function or method, but you're applying it to a non-function object of typeDecoratorClass.If you think this use case should be supported, the typing specification for
@overloadwould need to be extended to define the rules for how it should be handled. You're certainly welcome to start a thread in the Python typing discussion forums to see if you could gather support for such an extension.
And https://github.com/python/mypy/issues/16840#issuecomment-1918112477
[...] If we want to allow
@overloadto be used on callable objects — especially when those objects have overloaded signatures for__call__, then I think we'd need to extend the typing spec to accommodate this use case. As the typing spec is currently written, I wouldn't expect this to work.
Although from a very quick glance I still don't see anything regarding callables classes or __call__ in https://typing.python.org/en/latest/spec/overload.html
I personally think it would be beneficial if a type checker could automatically expand the overloads. In some bad cases it could explode to too many overloads real fast, but mypy already has a system of bailing out on too many overloads, so I don't see that as a blocker. I don't really have the motivation to start a discussion on the typing forums defending this idea though.
- VS Code with Pylance does not show the documentation of the function on hover only when the decorator is used
That's not something I had considered. Pylance already does a bit of magic to "forward" or "passthrough" docstrings from the real Python file if the docs have none.
one could argue that this should be discussed with the Pylance team instead.
I think either way it think it's worth looking into, but as you said, on Pylance's side and on their issue tracker.
If you have a particular task for someone familiar with networkx and Python typing, I have some spare cycles.
If you have a particular task for someone familiar with networkx and Python typing, I have some spare cycles.
Atm mostly lamenting functions decorated with @_dispatchable that we need to overload on top of that ^^" And discussing if we should take a different approach or some other action.
I think it's worth proposing to extend the typing spec, as Eric previously alluded to, to allow this use-case.
The following almost works and would sidestep the double overload issue entirely, but you loose keyword params entirely.
from collections.abc import Callable
from typing import Any, Generic, TypeVar
from typing_extensions import Self, TypeVarTuple, Unpack
_R = TypeVar("_R")
_Ts = TypeVarTuple("_Ts")
class _dispatchable(Generic[Unpack[_Ts], _R]):
def __new__(
cls,
func: Callable[_P, _R] | None = None,
# ...
) -> Self: ...
# Using @overload on __call__ also leads to issues where decorated methods can't also be overloaded
# https://github.com/python/typeshed/issues/14462#issuecomment-3212917747
# So we take the slight loss in type safety and IntelliSense for kwargs
def __call__(self, *args: Unpack[_Ts], backend: str | None = None, **extra_backend_kwargs: Any) -> _R: ...
It's a bit "pseudo-cody", but if the following would be possible:
from collections.abc import Callable
from typing import Any, Generic, TypeVar, TypedDict
from typing_extensions import ParamSpec, Self
_P = ParamSpec("_P")
_R = TypeVar("_R")
class _dispatchable(Generic[_P, _R]):
def __new__(
cls,
func: Callable[_P, _R] | None = None,
# ...
) -> Self: ...
def __call__(self, *args: _P.args, backend: str, **kwargs: _P.kwargs, **extra_backend_kwargs: Any ) -> _R: ...
# Or using the previously rejected keyword params concatenation, with a wishful use of TypedDict unpacking
# https://peps.python.org/pep-0612/#concatenating-keyword-parameters
class _BackendKWArgs(TypedDict, closed=False, total=False):
backend: str
class _dispatchable(Generic[_P, _R]):
def __new__(
cls,
func: Callable[_P, _R] | None = None,
# ...
) -> Self: ...
__call__: Callable[Concatenate[_P, ("backend", str), Unpack[_BackendKWArgs]], _R]