-
Notifications
You must be signed in to change notification settings - Fork 126
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
feat: has_operation #1807
Draft
camriddell
wants to merge
14
commits into
narwhals-dev:main
Choose a base branch
from
camriddell:feat/has_operation
base: main
Could not load branches
Branch not found: {{ refName }}
Loading
Could not load tags
Nothing to show
Loading
Are you sure you want to change the base?
Some commits from the old base branch may be removed from the timeline,
and old review comments may become outdated.
+403
β10
Draft
feat: has_operation #1807
Changes from all commits
Commits
Show all changes
14 commits
Select commit
Hold shift + click to select a range
333ccb6
add has_operation feature
camriddell c9ae6c1
add has_operation test
camriddell 4ce837b
fix has_operation False when backend module is missing
camriddell 574f62f
Merge branch 'main' into feat/has_operation
camriddell 9166fe3
refactor has_operation simplify type hints
camriddell b97d613
please mypy
camriddell 6b4a0ac
Merge branch 'main' of github.com:narwhals-dev/narwhals into feat/hasβ¦
camriddell 1d7b4b4
add SQLFrame implementation and test
camriddell 7a824cd
enh has_operation to check for simple NotImplemented cases
camriddell 956a12c
fix small test typo
camriddell fd40698
add utils.is_implemented raises node handling
camriddell 005574c
improve utils.get_class_... and utils.is_implemented coverage
camriddell 1777536
Merge branch 'main' of github.com:narwhals-dev/narwhals into feat/hasβ¦
camriddell 87b7114
align has_operation with new implementation changes
camriddell File filter
Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
There are no files selected for viewing
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Oops, something went wrong.
Oops, something went wrong.
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.
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'm not sure if this is the same, or just a similar issue - but reading this reminded me of an issue with
DType
.The way that the nested types work seems like its unsafe, but isn't flagged by a type checker.
Using
Array
as an example, it has attributes annotated in class-scope (but not as aClassVar
), without a default.narwhals/narwhals/dtypes.py
Lines 788 to 797 in 66628a5
Accessing on
type[Array]
would cause a runtime issue - but won't give a warning statically.narwhals/narwhals/_polars/utils.py
Lines 193 to 219 in 66628a5
For comparison,
Datetime
does provide warnings - and doesn't have class-scoped attributes:narwhals/narwhals/dtypes.py
Lines 491 to 500 in 66628a5
I guess what I'm thinking is having something like
polars
hasclassinstmethod
.But for properties that have a default (accessible on the type) - without leaking to instances.
I feel like this might be similar to what you have here
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.
pinging @MarcoGorelli to make sure you're aware of the potential bug here