Skip to content

TYP: make the type annotations of read_csv & read_table discoverable #34976

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

Merged
merged 3 commits into from
Jun 25, 2020

Conversation

topper-123
Copy link
Contributor

@topper-123 topper-123 commented Jun 24, 2020

In master the type of read_csv and read_table is 'Any'. This PR makes the functions signatures discoverable by mypy. Also fixes the error message when wrong kwarg is passed.

@topper-123 topper-123 force-pushed the read_csv_table_type branch from babb386 to ef4b0d2 Compare June 24, 2020 18:43
@WillAyd
Copy link
Member

WillAyd commented Jun 24, 2020

I am on board with this. This probably also closes #25648

@WillAyd WillAyd added the IO CSV read_csv, to_csv label Jun 24, 2020
@WillAyd
Copy link
Member

WillAyd commented Jun 24, 2020

So maybe worth pulling the test case from #33023

@WillAyd
Copy link
Member

WillAyd commented Jun 24, 2020

@gfyoung

@gfyoung
Copy link
Member

gfyoung commented Jun 24, 2020

I'm a bit torn on this TBH

I see the benefits to this change (mypy + #25648, #33023), but on the other hand, it does mean we will need to maintain two copies of essentially the same signature. This function generator setup was meant to alleviate that.

If we're mostly okay with maintaining two copies of the signature, then go for it. @pandas-dev/pandas-core

@gfyoung gfyoung added the Refactor Internal refactoring of code label Jun 24, 2020
@topper-123
Copy link
Contributor Author

@WillAyd , good idea taking in that test from #33023. I'll do that in the next commit.

@gfyoung , that was the reason I added the test test_read_table_same_signature_as_read_csv, to ensure the signatures don't drift apart :-)

@gfyoung
Copy link
Member

gfyoung commented Jun 25, 2020

@topper-123 : Ah, that's a fair point. Okay...it still feels little hacky, but the test does make me feel better about this.

@gfyoung gfyoung requested review from gfyoung and removed request for gfyoung June 25, 2020 01:35
Copy link
Member

@simonjayhawkins simonjayhawkins left a comment

Choose a reason for hiding this comment

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

Thanks @topper-123 lgtm.

engine = "c"
engine_specified = False

kwds.update(
Copy link
Member

Choose a reason for hiding this comment

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

at some point, would be beneficial for consistency checking to not use the dictionary here as mypy doesn't check this.

@jreback jreback added this to the 1.1 milestone Jun 25, 2020
Copy link
Contributor

@jreback jreback left a comment

Choose a reason for hiding this comment

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

@TomAugspurger
Copy link
Contributor

TomAugspurger commented Jun 25, 2020 via email

@WillAyd WillAyd merged commit a7d96fa into pandas-dev:master Jun 25, 2020
@WillAyd
Copy link
Member

WillAyd commented Jun 25, 2020

Thanks @topper-123

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
IO CSV read_csv, to_csv Refactor Internal refactoring of code
Projects
None yet
Development

Successfully merging this pull request may close these issues.

ERR: read_csv exposes an internal function when bad argument is specified
6 participants