Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
ENH: Allow compression in NDFrame.to_csv to be a dict with optional arguments (#26023) #26024
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
ENH: Allow compression in NDFrame.to_csv to be a dict with optional arguments (#26023) #26024
Changes from 22 commits
4e73dc4
ab7620d
2e782f9
83e8834
d238878
b41be54
60ea58c
8ba9082
0a3a9fd
a1cb3f7
af2a96c
5853a28
789751f
5b09e6f
68a2b4d
c856f50
8df6c81
40d0252
18a735d
103c877
b6c34bc
969d387
abfbc0f
04ae25d
9c22652
56a75c2
bbfea34
7717f16
779511e
780eb04
6c4e679
1b567c9
9324b63
7cf65ee
29374f3
6701aa4
0f5489d
e04138e
6f2bf00
865aa81
8d1deee
File filter
Filter by extension
Conversations
Jump to
There are no files selected for viewing
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
can you put the comment before the example (and put a blank line between cases); also might need to have a DOCTEST: SKIP here
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
can you do this
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think the Dict can be typed as
Dict[str, str]
here. Would also be nice to type the return though that may change depending on comment belowThere was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
can you do this
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
any -> Optional[str]
dict -> Dict[str, str]
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Was this throwing a Typing error? Think MyPy should be able to infer here
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@WillAyd Yeah, it was. Couldn’t tell why, but MyPy couldn’t infer when I added types to the function definition. It also occurred on
from pandas import Index
incsvs.py
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
What error was it giving you?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It was giving
pandas/io/common.py:367: error: Incompatible types in assignment (expression has type "Tuple[Type[BytesIO]]", variable has type "Tuple[Type[BytesIO], Any]")
only if any of the function's arguments have type annotations.There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
What error was this giving you?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
MyPy was giving
pandas/io/formats/csvs.py:125: error: Module 'pandas' has no attribute 'Index'
; It appears to only occur if there are type annotations on anyCSVFormatter
parameters.There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Make sure you try again on master - I think a separate PR should have resolved this already
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The same error appears on master if any type annotations are added.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@drew-heenan can you remove this and push as a new commit? Again thought we resolved this in a separate PR so would like to validate its not a mypy versioning thing between your local environment and what we have on CI
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@WillAyd Just did that - the error still appears.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Hmm OK thanks for confirming. @ryankarlos not sure if you have any insight - thought this would be resolved by #26019
@drew-heenan this isn't a blocker so OK to add back in the ignore I think; can review separate from this
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@WillAyd Got it, thanks for checking!
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Can you append
_raises
to this test name?There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Can you match on the expected message with
pytest.raises
here?There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
do the raises as an inner context manager,e .g.