Implement ADR 0002 (remove special handling of Optional and Union) #158
Conversation
SimonHeybrock
force-pushed
the
remove-optional-union
branch
from
bf26904
to
f55e818
Compare
SimonHeybrock
force-pushed
the
remove-optional-union
branch
from
f55e818
to
80b410b
Compare
| @@ -149,7 +149,7 @@ def _find_nodes_in_paths(graph: Graph, end: Key) -> List[Key]: | |||
|
|
|||
|
|
|||
| def _is_multiple_keys( | |||
| keys: type | Iterable[type] | Item[T], | |||
| keys: type | Iterable[type] | Item[T] | object, | |||
There was a problem hiding this comment.
Isn't this equivalent to keys: Any now? Why did you add object?
There was a problem hiding this comment.
Yes, I could not get it to work otherwise. I thought listing the others still serves as an expression of intent?
There was a problem hiding this comment.
Maybe. So did it not work with Any? From what I understand that is preferred over object.
There was a problem hiding this comment.
No, I mean I failed to find another more concrete type hint that works. Any would work.
src/sciline/pipeline.py
Outdated
| @@ -793,7 +752,13 @@ def compute(self, tp: Iterable[Type[T]], **kwargs: Any) -> Dict[Type[T], T]: | |||
| def compute(self, tp: Item[T], **kwargs: Any) -> T: | |||
| ... | |||
|
|
|||
| def compute(self, tp: type | Iterable[type] | Item[T], **kwargs: Any) -> Any: | |||
| @overload | |||
| def compute(self, tp: object, **kwargs: Any) -> Any: | |||
There was a problem hiding this comment.
Similar to above, this is equivalent to
| def compute(self, tp: object, **kwargs: Any) -> Any: | |
| def compute(self, tp: Any, **kwargs: Any) -> Any: |
if I'm not mistaken.
Does mypy use the specialised overloads where possible or does it always fall back to Any?
tests/pipeline_with_optional_test.py
Outdated
|
|
||
|
|
||
| def test_union_with_none_can_be_used_instead_of_Optional() -> None: | ||
| def test_Union_and_optional_are_distinct() -> None: |
There was a problem hiding this comment.
I don't think Union[T, None] and Optional[T] should be treated differently.
But this test doesn't actually seem to have anything to do with Optional. It just tests different argument orders.
There was a problem hiding this comment.
I don't think Union[T, None] and Optional[T] should be treated differently.
Then we would need to add special-case handling.
There was a problem hiding this comment.
Ah, now I think I understand what you want to test here. But there are no optionals in the test, only unions.
There was a problem hiding this comment.
Good catch, fixed I think!
|
We should probably change sciline/src/sciline/pipeline.py Line 358 in 5a8bdd9 Optional[Dict[Any, Any]] maybe with a comment about the weird behaviour of what is a type vs type hint?
|
|
I am thinking now that we should remove the |
Implement ADR 0001: Remove isinstance checks when setting params
While
Pipelinewill be re-implemented, I nevertheless made this change, since it allows for updating units tests with new expected behavior, which will make the re-implementation easier/smaller (will pass the same unit tests), decoupling the behavior change from the re-implementation.