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

Proposal for improving `GTData._replace` initialization #310

Closed jrycw closed 1 month ago

jrycw commented 2 months ago

In our codebase, GTData._replace is widely used. I'm suggesting two potential improvements for the initialization of GTData._replace:

codecov-commenter commented 2 months ago

Codecov Report

All modified and coverable lines are covered by tests :white_check_mark:

Project coverage is 81.71%. Comparing base (14ce2c5) to head (409f706).

Additional details and impacted files ```diff @@ Coverage Diff @@ ## main #310 +/- ## ======================================= Coverage 81.71% 81.71% ======================================= Files 41 41 Lines 4321 4321 ======================================= Hits 3531 3531 Misses 790 790 ```

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

machow commented 2 months ago

Thanks for looking into this---since GTData is a dataclass, we should be able to use the from dataclass import fields function to get field names. WDYT of that approach? IMO the biggest pain point of _replace() currently is that it doesn't have type hints, so the static checker doesn't warn when you use a kwarg that would error.

I think the _replace() method is mostly duplicating the dataclass replace() method. IIRC, it exists, because using replace() on the subclass GT.replace() doesn't quite work (due to GT using a totally different __init__() signature I think?).

It's sort of a balancing act between GTData (which we want for more limited scope in most our functions), and GT (where we want to have all the functions on it as methods).

jrycw commented 2 months ago

I tend to be more of an old-school Python developer and haven't used dataclass extensively in my code (although I know I should). However, after reviewing the codebase for the past few days, it appears that we've already utilized fields extensively. Based on my limited knowledge, it seems to be functioning well. For type hints, it appears that an adapter data structure like Enum or dataclass would provide a smoother coding experience.

machow commented 1 month ago

I'm going to close for now, but would love to revisit this in the future (and happy to reopen if useful:).