-
Notifications
You must be signed in to change notification settings - Fork 153
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
Suggestion: Removing singledispatch to enable static type checking #879
Suggestion: Removing singledispatch to enable static type checking #879
Conversation
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.
Thanks for making your first contributions. I think you intention of getting our annotations to work flawlessly is a good one. The execution however needs to be different.
The core issue is that you are changing the implementation to get type annotations. That is currently not our goal. I know that when we merged #401 we only implemented partial annotations (that was already a lot of work!).
We can avoid changing the implementation by keeping a separate __init__.pyi
file with all the annotations e.g.:
def distance_matrix(
graph: PyGraph[S, T] | PyDiGraph[S, T],
parallel_threshold:int=...,
as_undirected:bool=...,
null_value:float =...
)
def floyd_warshall(
graph: PyGraph[S, T] | PyDiGraph[S, T],
weight_fn: Option[Callable[T, float]],
default_weight:float = ...,
parallel_threshold:int = ...
): ...
That way we don't need to replace our current single dispatch implementation. It is perhaps not the cleanest but historically is what we used to glue the library together because it's not written in Python
S = TypeVar("S") | ||
T = TypeVar("T") | ||
|
||
class PyDAG(Generic[S, T], PyDiGraph[S, 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.
PyDAG is declared in __init__.py
already
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.
We need to keep this file as it will be the home of all annotations
null_value=null_value, | ||
) | ||
|
||
if isinstance(PyGraph, PyDiGraph): |
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 am pretty confident this is a typo
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.
Yep.
This makes sense, thanks for the reply. I got a bit focused on the idea that much of the code already is capable of being typed in its current form (with the above modification), but I get that it might be cheaper to just write type stubs. I'll close this, and if I find the time, I'll try to add some type stubs to |
This PR only touches Python code. It contains a small example of a fix that specifically lets the VSCode type checker (pyright / pylance) infer the types taken by functions, including showing docstrings.
What's the problem?
In the current main branch of rustworkx, VSCode is unable to correctly type (including docstrings) many functions that are present in init.py. The error looks like the following (using
distance_matrix
) as an example`:This is due to
__init__.pyi
, a type stub, that overshadows the definitions in__init__.py
. My first step was to merge these type stubs intorustworkx.pyi
, and delete__init__.pyi
. VSCode could now "see" thedistance_matrix
, but it's not very helpful to the end user:Jupyter doesn't have either of these problems, but I'd argue that the functionality offered by vscode is extremely valuable for future users of rustworkx.
With this PR, the single function now supports both PyDiGraph and PyGraph, using isinstance to decipher between the types (showing vscode and jupyter):
If we want to add different return types depending on inputs, we can use the
@overload
decorator to return different types depending on the input types. But that's probably not necessary for the majority of functions.If the behavior introduced by this PR is desired, I could spend some time on the other functions, and add them as well.