Uh oh!
There was an error while loading. Please reload this page.
POC: appease linter for gh-53 - #59
Conversation
| 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
Uh oh!
There was an error while loading. Please reload this page.
Uh oh!
There was an error while loading. Please reload this page.
Uh oh!
There was an error while loading. Please reload this page.
Uh oh!
There was an error while loading. Please reload this page.
Uh oh!
There was an error while loading. Please reload this page.
Uh oh!
There was an error while loading. Please reload this page.
Uh oh!
There was an error while loading. Please reload this page.
Uh oh!
There was an error while loading. Please reload this page.
Uh oh!
There was an error while loading. Please reload this page.
Uh oh!
There was an error while loading. Please reload this page.
Uh oh!
There was an error while loading. Please reload this page.
lucascolley
commented
Dec 10, 2024
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
crusaderky
commented
Dec 10, 2024
No need I can cherry-pick |
lucascolley
commented
Dec 10, 2024
Feel free to cherry-pick from this point @crusaderky and we can continue in your PR |
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.