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

Type hints for optional `None` values #312

Closed jrycw closed 2 months ago

jrycw commented 2 months ago

Currently, we employ three styles to describe parameters that might be assigned as None:

  1. Union[str, None]
  2. Optional[str]
  3. str | None

I personally prefer style 3. However, since this style is only supported in Python 3.10 and later, I'd like to gather the team's opinions on the preferred style. I'm interested in submitting a PR to address this issue in our codebase once we reach a conclusion.

machow commented 2 months ago

Hey! I feel onboard with option 3! I think the only place we can't currently do it is in _tbl_data.py, since functools.singledispatch does some dynamic type annotation inspection, which breaks in that case (I think this is only an issue in python 3.9, but could be wrong).

In any event, a refactor that did (3) anywhere possible would be a huge contribution! Thanks for taking time to emphasize the value of consistency in the codebase :)

jrycw commented 2 months ago

@machow Thank you for your response. Another question arises: Do you think we could use native data structures for type hints now? For instance, replacing typing.List with list.

machow commented 2 months ago

Yeah, I think that now we are 3.9+, using list[...] should work. AFAICT the easiest way to check is to look up the python docs entry for the corresponding typing class, and see if there's a deprecation note:

https://docs.python.org/3/library/typing.html#typing.List

It looks like typing.Optional was deprecated in 3.10, but afaik as long as you have from __future__ import annotations, you should be able to use x | None. Or at the very least, use it wrapped in a string? "x | None"

jrycw commented 2 months ago

Thank you for pointing out the related docs. I'll start working on a PR to update our type hints using the new style.

jrycw commented 2 months ago

I'll go ahead and close this discussion since the resolution has been implemented in the merged PR #315.