Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
POC: appease linter for gh-53 #59
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
POC: appease linter for gh-53 #59
Changes from 5 commits
806f037
aa0d364
de00bde
bea4427
7ae5766
55a039a
File filter
Filter by extension
Conversations
Jump to
There are no files selected for viewing
Large diffs are not rendered by default.
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.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I agree
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.
looks like this is no longer needed
EDIT: only if
at
is madefinal
Check warning on line 675 in src/array_api_extra/_funcs.py
src/array_api_extra/_funcs.py#L671-L675
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.
is it perhaps possible to
@overload
these cases?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.
It isn't because the return type depends on a duck-type test on x. And I'm definitely unwilling to explore writing a
class HasAtMethod(Protocol)
for a small internal function that is consumed exclusively 2 paragraph below.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 willing, so here you go:
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.
making this change - could you clarify how to use
_CanAt
@jorenham ?Check warning on line 695 in src/array_api_extra/_funcs.py
src/array_api_extra/_funcs.py#L695
Check warning on line 703 in src/array_api_extra/_funcs.py
src/array_api_extra/_funcs.py#L703
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.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
but I agree with mypy and pyright that the
else
clause here is redundantThere 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.
you could alternatively move the
if copy is None
to the top, and then doelif copy: ... else: ...
. That way you'd also allow e.g.0
,1
, and thenp.bool_
values (at runtime)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.
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 explicitcopy
andxp
kwargsCheck warning on line 737 in src/array_api_extra/_funcs.py
src/array_api_extra/_funcs.py#L736-L737
Check warning on line 745 in src/array_api_extra/_funcs.py
src/array_api_extra/_funcs.py#L744-L745
Check warning on line 766 in src/array_api_extra/_funcs.py
src/array_api_extra/_funcs.py#L765-L766
Check warning on line 770 in src/array_api_extra/_funcs.py
src/array_api_extra/_funcs.py#L770
Check warning on line 778 in src/array_api_extra/_funcs.py
src/array_api_extra/_funcs.py#L778
Check warning on line 802 in src/array_api_extra/_funcs.py
src/array_api_extra/_funcs.py#L802