Refactoring: Clarifying the responsabilities of each component #102
Conversation
There was a problem hiding this comment.
Hey @gabrieldemarmiesse! Thanks for this PR!! Your changes make a lot of sense. I've left a few comments and suggestions. :)
| self, | ||
| method: Optional[Callable[..., Any]] = None, | ||
| precedence: int = 0, | ||
| ) -> Union[Function, Callable[[Callable[..., Any]], Function]]: |
There was a problem hiding this comment.
@gabrieldemarmiesse I wonder if this is the more accurate way to type what @dispatch does. What do you think of something like the following?
from typing import TypeVar, Protocol
F = TypeVar("F")
class FunctionProtocol(Function, Protocol[F]):
__call__: F
...
@overload
def __call__(self, method: None, precedence: int = 0) -> Union[
Callable[[F], FunctionProtocol[F]],
Callable[[F, int], FunctionProtocol[F]],
]:
...
def __call__(self, method: F, precedence: int = 0) -> FunctionProtocol[F]:
...I'm not sure if that actually works, but it might better capture the spirit of @dispatch: retain what the function does originally, but add Function-functionality to it.
| try: | ||
| self._resolve_pending_registrations() | ||
| except NameError: # pragma: specific no cover 3.7 3.8 3.9 | ||
| # When `staticmethod` is combined with | ||
| # `from __future__ import annotations`, in Python 3.10 and higher | ||
| # `staticmethod` will attempt to inherit `__doc__` (see | ||
| # https://docs.python.org/3/library/functions.html#staticmethod). Since | ||
| # we are still in class construction, forward references are not yet | ||
| # defined, so attempting to resolve all pending methods might fail with a | ||
| # `NameError`. This is fine, because later calling `__doc__` on the | ||
| # `staticmethod` will again call this `__doc__`, at which point all methods | ||
| # will resolve properly. For now, we just ignore the error and undo the | ||
| # partially completed :meth:`Function._resolve_pending_registrations` by | ||
| # clearing the cache. | ||
| self.clear_cache(reregister=False) |
There was a problem hiding this comment.
Could you explain why you've removed this? Do you think it's not at all necessary to resolve pending registration when getting the owner? I should've documented why that line was there in the first place... I think removing it might make sense, but I'm worried about subtly breaking something that's currently not tested for, so I just want to double check with you.
| assert len(g.methods) == 1 | ||
| assert g.methods[0].implementation == f |
There was a problem hiding this comment.
| assert len(g.methods) == 1 | |
| assert g.methods[0].implementation == f | |
| assert len(g.methods) == 1 | |
| assert g.methods[0].implementation == f | |
| assert g.methods[0].precedence == 0 |
| assert len(f._pending) == 0 | ||
| assert len(f._cache) == 0 | ||
| f._resolver | ||
| assert f._methods_registry._resolver._cache == {} |
There was a problem hiding this comment.
| assert f._methods_registry._resolver._cache == {} | |
| assert not f._methods_registry._resolver._cache |
(slightly more Pythonic)
| self.signatures: List[Signature] = [] | ||
| self.is_faithful: bool = True | ||
| def __init__(self, signatures: Iterable[Signature]): | ||
| signatures_dict = {hash(s): s for s in signatures} |
There was a problem hiding this comment.
Ahhh, I now understand that this is used to remove duplicate signatures. What I'm worried about is specifically which duplicate is kept. Specifically, if signatures implements the same method twice, then the later one should be kept. Right now, which implementation is kept is up to the implementation of the dictionary. I think it's safest to implement this in way that very explicitly retains the later implementations.
There was a problem hiding this comment.
We should also have this tested by a unit test!
|
|
||
| def resolve(self, target: Union[Tuple[object, ...], Signature]) -> Signature: | ||
| def _resolve(self, target: Union[Tuple[object, ...], Signature]) -> Signature: |
| with pytest.raises(NotFoundLookupError): | ||
| # E: pyright(argument of type "float" cannot be assigned to parameter "x") | ||
| # E: mypy(no overload variant of "f" matches argument type "float") | ||
| f(1.0) | ||
| f(1.0, 2.0) |
There was a problem hiding this comment.
| f(1.0, 2.0) | |
| f(1.0, 2.0) | |
| def test_methods() -> None: | |
| assert f.methods |
How about adding in a unit test like this? That would test whether the typing for Dispatcher is right.
Thus we simplify the whole logic of list swapping, cache invalidation and such. The resolver is fully in charge of doing whatever it sees fit with the inputs, without having other concepts getting in the way.
If one is brave enough, by changing only the Signature class + the resolver, it should be enough to support keyword arguments.
Closes #92