Codebase cleanup and minor improvements #305
Conversation
remove unused imports
remove unused imports
remove unused imports
fix unpacking
fix unpacking
fix return
fix isinstance
remove unused imports
fix isinstance
fix isinstance
remove unused import
`location` should be `locations`
remove unused import
remove unused import
Codecov ReportAttention: Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #305 +/- ##
==========================================
+ Coverage 81.66% 81.88% +0.21%
==========================================
Files 41 41
Lines 4276 4256 -20
==========================================
- Hits 3492 3485 -7
+ Misses 784 771 -13 ☔ View full report in Codecov by Sentry. |
fix the wrong logic of isinstance
revert isinstance style for `_assert_str_scalar` and `_assert_str_list`
simplify the logic of `_unique_set`
revert the logic of `_unique_set`
great_tables/gt.py
Outdated
| @@ -406,7 +404,7 @@ def _render_as_html( | |||
| return finalized_table | |||
|
|
|||
| def _finalize_html_table( | |||
| style: str, quarto_disable_processing: str, quarto_use_bootstrap: str, *args: Any | |||
| self, style: str, quarto_disable_processing: str, quarto_use_bootstrap: str, *args: Any | |||
There was a problem hiding this comment.
cc @rich-iannone If this is unused, I think we can take it out (but can also punt that to a later PR :)
There was a problem hiding this comment.
Yes, this is entirely unused. Let's remove it in this PR.
great_tables/utils_render_common.py
Outdated
| @@ -11,7 +11,7 @@ | |||
|
|
|||
| def get_row_reorder_df(groups: RowGroups, stub_df: Stub) -> list[TupleStartFinal]: | |||
| # Get the number of non-None entries in the `groupname_col` | |||
| n_stub_entries = len([entry for entry in stub_df if entry.group_id is not None]) | |||
| n_stub_entries = sum(1 for entry in stub_df if entry.group_id is not None) | |||
There was a problem hiding this comment.
I'm a fan of len(), since it's a strong indicator that the length of something is being taken (whereas sum in combo with 1s is two elements that together do something similar).
(just my take; leaving to y'all :)
There was a problem hiding this comment.
@machow it's merely a coding preference. Please feel free to revert back to using len, as it might contribute to a more consistent codebase.
There was a problem hiding this comment.
I'm also in agreement with keeping this with len()
There was a problem hiding this comment.
LGTM, thanks! I left a quick thought about using sum(1...), but am okay with it being used :).
@rich-iannone do you want to take a look (if useful) and merge?
|
Thank you @jrycw for all the work done in this PR! Will be merging this now. |
This PR primarily focuses on cleaning up our codebase, largely based on the discussion in #302. However, during my review of the code, I also identified several minor areas for improvement, which I've addressed in this PR.
These modifications may contain some subjective changes. I kindly ask the team to review them based on your judgment. Any feedback or comments would be greatly appreciated.