posit-dev / great-tables

Make awesome display tables using Python.
https://posit-dev.github.io/great-tables/
MIT License
1.43k stars 48 forks source link

Update type hints and organize import modules #315

Closed jrycw closed 2 months ago

jrycw commented 2 months ago

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 commented 2 months ago

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 :warning:
great_tables/_types.py 0.00% 2 Missing :warning:
great_tables/_body.py 50.00% 1 Missing :warning:
great_tables/_locations.py 90.90% 1 Missing :warning:
great_tables/_utils_render_html.py 90.90% 1 Missing :warning:
Additional details and impacted files ```diff @@ 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 ```

:umbrella: View full report in Codecov by Sentry.
:loudspeaker: Have feedback on the report? Share it here.

jrycw commented 2 months ago

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

jrycw commented 2 months ago

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

jrycw commented 2 months ago

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 commented 2 months ago

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 commented 2 months ago

@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.

jrycw commented 2 months ago

Fix small pre-commit issues by 5c3fe2c.

rich-iannone commented 2 months ago

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

machow commented 2 months ago

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