Skip to content
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

GH:624 - added mode keyword to DataFrame.to_json #684

Merged
merged 7 commits into from
May 11, 2023
Merged

GH:624 - added mode keyword to DataFrame.to_json #684

merged 7 commits into from
May 11, 2023

Conversation

ramvikrams
Copy link
Contributor

@ramvikrams ramvikrams commented May 5, 2023

Copy link
Collaborator

@Dr-Irv Dr-Irv left a comment

Choose a reason for hiding this comment

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

This is a bit tricky. You have to put the 2 overloads for orient = Literal["records"] first, but without the "= ..." . The first two overloads would then have lines: Literal[True] and mode: Literal["a"] . This may need an * in the right places (see DataFrame.to_dict() for an example). Then you add another pair of overloads with orient: Literal["records"] and lines: Literal[False] = ..., mode: Literal["w"] = .... Then you have the 2 overloads that are there where mode: Literal["w"] = ... is allowed.

You also have to fix the overloads in series.pyi.

For the tests, you should test the valid and invalid combinations, using TYPE_CHECKING_INVALID_USAGE for the invalid ones, with appropriate ignore statements.

pandas-stubs/core/frame.pyi Outdated Show resolved Hide resolved
pandas-stubs/core/frame.pyi Outdated Show resolved Hide resolved
pandas-stubs/core/frame.pyi Outdated Show resolved Hide resolved
pandas-stubs/core/frame.pyi Outdated Show resolved Hide resolved
pandas-stubs/core/frame.pyi Outdated Show resolved Hide resolved
pandas-stubs/_typing.pyi Outdated Show resolved Hide resolved
@ramvikrams
Copy link
Contributor Author

ramvikrams commented May 8, 2023

Then you add another pair of overloads with orient: Literal["records"] and lines: Literal[False] = ..., mode: Literal["w"] = ....

Were you trying to write mode: Literal["a"] = ... , but it is not valid i think and if it is mode: Literal["w"] then in the last two overloads for mode: Literal["w"] we would have to do lines:[True] in the last two overloads and check lines separately, otherwise the overloads will overlap. As we have added records back to JsonFrameOrient as you had suggested in the review.

@Dr-Irv
Copy link
Collaborator

Dr-Irv commented May 8, 2023

Then you add another pair of overloads with orient: Literal["records"] and lines: Literal[False] = ..., mode: Literal["w"] = ....

Were you trying to write mode: Literal["a"] = ... , but it is not valid i think and if it is mode: Literal["w"] then in the last two overloads for mode: Literal["w"] we would have to do lines:[True] in the last two overloads and check lines separately, otherwise the overloads will overlap. As we have added records back to JsonFrameOrient as you had suggested in the review.

It may be that the second set of overloads I suggested are not needed because of the overlap you mention.

@ramvikrams
Copy link
Contributor Author

ramvikrams commented May 10, 2023

I have done the needfull but the only problem is with the warnings in the test, could you please help with that

pandas-stubs/core/series.pyi Outdated Show resolved Hide resolved
tests/test_frame.py Outdated Show resolved Hide resolved
tests/test_frame.py Outdated Show resolved Hide resolved
tests/test_frame.py Outdated Show resolved Hide resolved
Copy link
Collaborator

@Dr-Irv Dr-Irv left a comment

Choose a reason for hiding this comment

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

2 small changes, and then it should be good to go

tests/test_frame.py Outdated Show resolved Hide resolved
tests/test_series.py Outdated Show resolved Hide resolved
Copy link
Collaborator

@Dr-Irv Dr-Irv left a comment

Choose a reason for hiding this comment

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

2 small changes, and then it should be good to go

Copy link
Collaborator

@Dr-Irv Dr-Irv left a comment

Choose a reason for hiding this comment

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

Thanks @ramvikrams

@Dr-Irv Dr-Irv merged commit 621891b into pandas-dev:main May 11, 2023
@ramvikrams ramvikrams deleted the t15 branch May 11, 2023 17:24
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Version 2.0 Compatibility Tracker
2 participants