-
-
Notifications
You must be signed in to change notification settings - Fork 1.6k
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
refactor(python): InterchangeDataFrame.version
should be a ClassVar
(not a property
)
#16312
Conversation
…int`) to be consistent with `InterchangeDataFrame`
Codecov ReportAll modified and coverable lines are covered by tests ✅
Additional details and impacted files@@ Coverage Diff @@
## main #16312 +/- ##
========================================
Coverage 81.33% 81.33%
========================================
Files 1403 1403
Lines 183293 183463 +170
Branches 2924 2929 +5
========================================
+ Hits 149085 149226 +141
- Misses 33705 33734 +29
Partials 503 503 ☔ View full report in Codecov by Sentry. |
On the Protocol it needs to be a In fact, in the official reference it's a regular property, so we should follow that as closely as possible: If there is something not working correctly, please make an issue to discuss and I may reopen this. |
…r` (not a `property`)
PolarsDataFrame.version
is a property
(not an int
) to be consistent with InterchangeDataFrame
InterchangeDataFrame.version
should be a ClassVar
(not a property
)
I'm sorry, I don't understand how The issue is that replacing a Mypy does not enforce this consistency unless the parent class explicitly declares the attribute as a ClassVar (pyright flags it always). edit: since github does not render new commits for closed PRs, here is how the ClassVar version would look like: |
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.
You're right, ClassVar is the way to go here. Thanks for the PR and the explanation!
No description provided.