Skip to content
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 GT.opt_all_caps() #436

Open
wants to merge 3 commits into
base: main
Choose a base branch
from
Open

Conversation

jrycw
Copy link
Contributor

@jrycw jrycw commented Sep 11, 2024

I'm curious if GT.opt_all_caps() can be refactored as suggested in this PR.

Copy link

codecov bot commented Sep 11, 2024

Codecov Report

Attention: Patch coverage is 89.47368% with 2 lines in your changes missing coverage. Please review.

Project coverage is 86.88%. Comparing base (bacda7e) to head (f0ae1a7).
Report is 6 commits behind head on main.

Files with missing lines Patch % Lines
great_tables/_options.py 88.88% 2 Missing ⚠️
Additional details and impacted files
@@            Coverage Diff             @@
##             main     #436      +/-   ##
==========================================
+ Coverage   86.37%   86.88%   +0.50%     
==========================================
  Files          42       42              
  Lines        4683     4674       -9     
==========================================
+ Hits         4045     4061      +16     
+ Misses        638      613      -25     

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

@jrycw jrycw force-pushed the update-opt-all-caps branch 2 times, most recently from b1197c4 to d932df5 Compare September 12, 2024 10:41
@jrycw
Copy link
Contributor Author

jrycw commented Sep 12, 2024

@rich-iannone and @machow , sorry for the messy commits.

The reason for this is that while trying to modify the locations parameter to favor Loc objects, a circular import issue occurred.

Currently, the type annotation and initial value of locations are incorrect, causing the documentation build to fail in CI. Since this is a breaking change, I would like to get your feedback on this proposed update.

@machow
Copy link
Collaborator

machow commented Sep 12, 2024

Hey, thanks for working on this -- this was a good nudge that we should probably circle back to #341 and round out the support of loc pieces in Great Tables. I can pick up #341 next week, so we can merge these changes in around the same time :).

@jrycw
Copy link
Contributor Author

jrycw commented Sep 13, 2024

Thanks for bringing up the related issue. Initially, I was thinking of using a Loc object like loc.column_labels for the locations parameter. However, using a Loc instance like loc.column_labels() seems like a better idea.

@machow
Copy link
Collaborator

machow commented Oct 4, 2024

Sorry for the wait, now that the new location stuff is merged in, I can take a look at this next week

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants