Refactor GT.opt_all_caps()#436
Conversation
Codecov Report❌ Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #436 +/- ##
==========================================
- Coverage 92.35% 92.31% -0.04%
==========================================
Files 48 48
Lines 6040 6039 -1
==========================================
- Hits 5578 5575 -3
- Misses 462 464 +2 ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
jrycw
force-pushed
the
update-opt-all-caps
branch
2 times, most recently
from
b1197c4 to
d932df5
Compare
jrycw
force-pushed
the
update-opt-all-caps
branch
from
d932df5 to
f0ae1a7
Compare
|
@rich-iannone and @machow , sorry for the messy commits. The reason for this is that while trying to modify the Currently, the type annotation and initial value of |
|
Thanks for bringing up the related issue. Initially, I was thinking of using a |
|
Sorry for the wait, now that the new location stuff is merged in, I can take a look at this next week |
machow
left a comment
machow
left a comment
There was a problem hiding this comment.
This LGTM, I did a quick scan -- but based on our pairing feel good about it. I left a quick note about making a type more specific. Feel free to add here and merge, or punt on it!
| self: GTSelf, | ||
| all_caps: bool = True, | ||
| locations: str | list[str] = ["column_labels", "stub", "row_group"], | ||
| locations: type[Loc] | list[type[Loc]] | str | list[str] | None = None, |
There was a problem hiding this comment.
I wonder if type[Loc] could be more specific? Since some types of Loc can't be passed.
list[Literal[LocColumnLabels, LocRowGroups, LocStub]]
There was a problem hiding this comment.
Found that list[Literal[LocColumnLabels, LocRowGroups, LocStub]] won't work because Literal is for values like strings/ints, but this simimlar approach works:
type[LocColumnLabels] | type[LocRowGroups] | type[LocStub] | list[type[LocColumnLabels] | type[LocRowGroups] | type[LocStub]] | ...
| if invalid: | ||
| raise ValueError(f"Invalid location string: {invalid[0]!r}") | ||
| locations = [_str_to_loc[x] for x in locations] | ||
| warnings.warn( |
There was a problem hiding this comment.
Can you put the warning at the top of the if any(...), so people will see it whether or not this errors?
3 participants
I'm curious if
GT.opt_all_caps()can be refactored as suggested in this PR.