-
Notifications
You must be signed in to change notification settings - Fork 106
feat: implement cols_reorder()
for flexible column reordering
#625
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
base: main
Are you sure you want to change the base?
Conversation
Codecov ReportAttention: Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #625 +/- ##
==========================================
+ Coverage 90.85% 90.86% +0.01%
==========================================
Files 46 46
Lines 5423 5432 +9
==========================================
+ Hits 4927 4936 +9
Misses 496 496 ☔ View full report in Codecov by Sentry. |
great_tables/_spanners.py
Outdated
function: `cols_reorder()`. By passing a list of column names to the `columns=` parameter, | ||
the table will be reordered accordingly: | ||
```{python} | ||
col1, col2, col3, *cols = exibble.columns # col1="num", col2="char", col3="fctr" |
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 is a really good example!
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've renamed the variables to use their actual column names instead of generic names like col1
. This should improve readability.
|
||
if isinstance(columns, str): | ||
columns = [columns] | ||
|
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 wonder if we should raise here, if only to provide a more user-friendly message. The user will instead see "Reordering vars must contain all boxhead vars."
if not providing all columns, which is good for a developer-facing error message but it exposes some of the underlying implementation that the user might not know about.
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.
Great suggestion! Should we raise the exception inside the function or modify the error message to be more user-friendly rather than developer-focused?
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.
Good question. I’m for slightly tweaking the wording in the existing location. Just switch vars
with columns
and avoid the use of boxhead
.
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.
How about "Column reordering must include all columns in the table."
?
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 like it!
Hello team,
This PR introduces
cols_reorder()
, allowing users to reorder columns in a specified order easily.While the existing
move_*()
functions are useful for repositioning a few columns, they don't provide fine-grained control over column order. I've often had to rely onPandas
orPolars
for this task. The implementation of this function follows a straightforward approach, similar to the existingmove_*()
functions.Feedback is always welcome!😊