Conversation
src/array_api_extra/_funcs.py
Outdated
| elif copy is None: # type: ignore[redundant-expr] | ||
| writeable = is_writeable_array(x) | ||
| copy = _is_update and not writeable | ||
| else: | ||
| msg = f"Invalid value for copy: {copy!r}" # type: ignore[unreachable] # pyright: ignore[reportUnreachable] |
There was a problem hiding this comment.
is there a better way to deal with situations like this where invalid types can be passed at runtime?
There was a problem hiding this comment.
This double-ignore is a strong example of why I'm strongly in favour of having only one type checker. This is tedious to both read and write.
There was a problem hiding this comment.
it's definitely annoying, but unfortunately it's the only possibility for ensuring compatibility with these two main type-checkers
There was a problem hiding this comment.
but I agree with mypy and pyright that the else clause here is redundant
There was a problem hiding this comment.
you could alternatively move the if copy is None to the top, and then do elif copy: ... else: .... That way you'd also allow e.g. 0, 1, and the np.bool_ values (at runtime)
There was a problem hiding this comment.
what if we still want to throw an error at runtime if copy="foo" is passed? I guess the argument is that, since this is internal, that would always be caught by the static analysis anyway?
EDIT: ah, it isn't internal, since the methods pass**kwargs through. I suppose this can be resolved once the methods are given explicit copy and xp kwargs
|
broadly, I agree on not going overkill on the types. Let's just get something in that works. |
|
|
||
| x: Array | ||
| idx: Index | ||
| __slots__: ClassVar[tuple[str, str]] = ("idx", "x") |
There was a problem hiding this comment.
IMHO the linter should not force me to define the type of __slots__, because it's part of the python data model. This only adds attrition and reduces readability.
There was a problem hiding this comment.
looks like this is no longer needed
EDIT: only if at is made final
No need I can cherry-pick |
|
Feel free to cherry-pick from this point @crusaderky and we can continue in your PR |
3 participants
Looks like there are no unresolvable or essential conflicts here, just required a little bit of wrangling :)
@jorenham would you be able to review the typing? I've added a few
Anyaliases where I wasn't sure how to annotate.@crusaderky once we check that the linter is happy here, shall I make a PR to your branch? Then you can continue your work in gh-53.