-
Notifications
You must be signed in to change notification settings - Fork 50
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: add Table.delete_row
method
#610
Conversation
Codecov ReportAttention:
Additional details and impacted files@@ Coverage Diff @@
## main #610 +/- ##
==========================================
- Coverage 87.75% 87.73% -0.02%
==========================================
Files 40 40
Lines 4705 4722 +17
==========================================
+ Hits 4129 4143 +14
- Misses 576 579 +3
☔ View full report in Codecov by Sentry. |
else: | ||
indices.add(index) | ||
if header is not None: | ||
if isinstance(header, str) or not isinstance(header, Sequence): |
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.
Oh man, I needed a double-take to understand why this wasn't just not isinstance(header, Sequence)
. That's annoying 😂
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.
Yep, classic :) string is a sequence but not "that" kind :)
Missing a couple of test coverage lines, but looks good to me. I basically agree with your assessment. I wouldn't look to the pandas API but the draft DataFrame API, which bizarrely has |
Thanks! Btw: I would not assume that it's in place because of See this type of usage for example: https://peps.python.org/pep-0673/#use-in-classmethod-signatures |
Fascinating! Thanks for the info! |
cc @jni
would appreciate your input on the API. I considered using
Table.drop
like the pandas API, but I thought it might be confusing that this one is in-place and that one is not. I'm also wary of the confusion between row indices and row headers (both of which could conceivably be integers), so I forced the usage of a kwarg.