Skip to content

fix: move literals to types.py #672

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

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

Conversation

jrycw
Copy link
Collaborator

@jrycw jrycw commented Apr 23, 2025

Hello team,

This PR provides an initial implementation for #526.

Copy link

codecov bot commented Apr 23, 2025

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 91.18%. Comparing base (ae0204c) to head (0b79b30).

Additional details and impacted files
@@            Coverage Diff             @@
##             main     #672      +/-   ##
==========================================
+ Coverage   91.17%   91.18%   +0.01%     
==========================================
  Files          47       48       +1     
  Lines        5462     5471       +9     
==========================================
+ Hits         4980     4989       +9     
  Misses        482      482              

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

🚀 New features to boost your workflow:
  • ❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.

Copy link
Member

@rich-iannone rich-iannone left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM!

@jrycw
Copy link
Collaborator Author

jrycw commented Apr 25, 2025

Could the team help review which literals (if not all) should be included in the __all__ in __init__.py?

@jrycw
Copy link
Collaborator Author

jrycw commented Apr 26, 2025

Initially, I considered exposing all the literals directly in __init__, but I realized it might be better to expose the types instead, similar to what we did with the vals module.

@machow
Copy link
Collaborator

machow commented May 6, 2025

Thanks for this PR -- we're actively discussing types right now. No need to update this PR right now. I'll be sure to come back with more fleshed out thoughts, if that's okay!

Background

We'll be working on a extension package for Great Tables (with extra nanoplots, themes, etc.. that we don't want to load up the main package too hard with), so this piece will be super relevant.

Two big pieces for us to think about:

  • Organizing types: how should type annotations be organized and communicated in types.py?
  • Exposing types: do we want a public types module right now?

I feel like the polars._types submodule has been a helpful reference. It looks like they deprecated their polars.type_aliases submodule for now, so it might be good to marinate this feature for a bit.

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.

3 participants