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

Update type hints and organize import modules #315

Merged
merged 38 commits into from
May 2, 2024
Merged

Conversation

jrycw
Copy link
Contributor

@jrycw jrycw commented Apr 25, 2024

This PR, related to #312, is aimed at updating the type hints of our codebase using newer syntax. During this process, I believe it might be beneficial to organize the import modules using #119 isort.

I hope this PR will lay a solid foundation for us to adopt more advanced techniques of type hints in the future.

@codecov-commenter
Copy link

codecov-commenter commented Apr 25, 2024

Codecov Report

Attention: Patch coverage is 97.61905% with 7 lines in your changes are missing coverage. Please review.

Project coverage is 82.87%. Comparing base (b4f5ad9) to head (5c3fe2c).

Files Patch % Lines
great_tables/_options.py 66.66% 2 Missing ⚠️
great_tables/_types.py 0.00% 2 Missing ⚠️
great_tables/_body.py 50.00% 1 Missing ⚠️
great_tables/_locations.py 90.90% 1 Missing ⚠️
great_tables/_utils_render_html.py 90.90% 1 Missing ⚠️
Additional details and impacted files
@@            Coverage Diff             @@
##             main     #315      +/-   ##
==========================================
- Coverage   82.89%   82.87%   -0.02%     
==========================================
  Files          41       41              
  Lines        4292     4287       -5     
==========================================
- Hits         3558     3553       -5     
  Misses        734      734              

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

@jrycw
Copy link
Contributor Author

jrycw commented Apr 25, 2024

Well, it seems that even after adding from __future__ import annotations, Python 3.9 still doesn't like the | syntax.

@jrycw
Copy link
Contributor Author

jrycw commented Apr 25, 2024

The Python 3.9 issue appears to be resolved by adding quotes to certain variables, as seen in
45b3f19
.

@jrycw
Copy link
Contributor Author

jrycw commented Apr 26, 2024

By the way, I've noticed that there are some modules imported in each section of _gt_data.py, and some of them are duplicated. Is this intentional, or can we organize all the modules at the top?

@machow machow self-requested a review April 29, 2024 21:21
@machow
Copy link
Collaborator

machow commented Apr 29, 2024

Thanks so much for this giant cleanup PR!

By the way, I've noticed that there are some modules imported in each section of _gt_data.py, and some of them are duplicated. Is this intentional, or can we organize all the modules at the top?

I think we can organize at top now. Originally, that file was split into many pieces, and we moved it into one as a refactor, but didn't get to cleaning up redundant imports 😓.

Are you using a tool like isort or ruff's sorting to sort the imports?

In any event, if you're happy with the PR as is, I'm happy to review / merge! (Can always figure out using isort/ruff sorting after merging)

@jrycw
Copy link
Contributor Author

jrycw commented Apr 30, 2024

@machow I've re-organized the import modules at the top for _gt_data.py. Could you please review/merge this PR?

Personally I've started using ruff to replace isort and black, which seems promising to me. However, there are many parameters that can be tweaked. For this PR, I manually ran isort for each file while editing, but please note that the final result may be affected by the pre-commit hook.


if TYPE_CHECKING:
from great_tables._types import GTSelf


RGBColor: TypeAlias = tuple[int, int, int]
Copy link
Collaborator

Choose a reason for hiding this comment

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

This is great

Copy link
Collaborator

@machow machow left a comment

Choose a reason for hiding this comment

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

This is really great, thanks for bringing our annotations into the python 3.9+ world!

I left a note wondering there's a better name for X in _format_vals.py. But this module isn't heavily used, so it seems okay to punt!


if TYPE_CHECKING:
from ._formats import DateStyle, TimeStyle
from ._tbl_data import SeriesLike


def _make_one_col_table(vals: Union[Any, List[Any], SeriesLike]) -> GT:
X: TypeAlias = "Any | list[Any] | SeriesLike"
Copy link
Collaborator

@machow machow May 1, 2024

Choose a reason for hiding this comment

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

We might want to give this a more informative name? (Or maybe not?)

edit: seems okay as is for now

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@machow I hope you can bear with me as I couldn't find a more suitable name. I used X because it appears that the first parameter of all val_* functions is named x.

@@ -702,8 +699,8 @@ class SpannerInfo:
spanner_label: str | None = None
spanner_units: str | None = None
spanner_pattern: str | None = None
vars: list[str] = field(default_factory=lambda: [])
built: Optional[str] = None
vars: list[str] = field(default_factory=list)
Copy link
Collaborator

Choose a reason for hiding this comment

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

great catch 😅

@jrycw
Copy link
Contributor Author

jrycw commented May 1, 2024

Fix small pre-commit issues by 5c3fe2c.

@rich-iannone
Copy link
Member

@machow this all looks great to me. If you wanted to merge @jrycw ‘s good work here, please feel free to do so!

@machow machow merged commit aa14ccf into posit-dev:main May 2, 2024
9 checks passed
@machow
Copy link
Collaborator

machow commented May 2, 2024

Sorry about the formatting issue in the merge commit, and thanks for all this helpful work!

@jrycw jrycw deleted the fix-312 branch May 2, 2024 03:50
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.

None yet

4 participants