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

Fix to_dict and from_dict type stubs #39

Merged
merged 6 commits into from
Jul 19, 2022

Conversation

KianShah
Copy link
Contributor

No description provided.

@KianShah
Copy link
Contributor Author

KianShah commented Jun 23, 2022

There seems to be a difference between the from_dict stub in the https://github.com/VirtusLab/pandas-stubs repo vs the stub here. There, they use the Orientation type in https://github.com/VirtusLab/pandas-stubs/blob/master/third_party/3/pandas/_typing.pyi, which obviously needs to be changed. Need some guidance from more senior contributors on where we need to make the change. I know that you guys are in the process of migrating to this repo, so it may not be an easy question to answer

@Dr-Irv
Copy link
Collaborator

Dr-Irv commented Jun 23, 2022

There seems to be a difference between the from_dict stub in the https://github.com/VirtusLab/pandas-stubs repo vs the stub here. There, they use the Orientation type in https://github.com/VirtusLab/pandas-stubs/blob/master/third_party/3/pandas/_typing.pyi, which obviously needs to be changed. Need some guidance from more senior contributors on where we need to make the change. I know that you guys are in the process of migrating to this repo, so it may not be an easy question to answer

The stubs here evolved from work done by Microsoft. These stubs try to match what is in the pandas source (and there is more work to do to make that happen). The VirtusLab stubs were done independently, but after discussion between them, Microsoft and the pandas team, we went forward with the Microsoft stubs as they were more complete. In the case of to_dict and from_dict, in the pandas source, we haven't created a type to represent the possible orient arguments, so it's fine to just use the Literal designation here. Also, the orient arguments are asymmetric in the two methods, so having a type to represent it doesn't help much.

Having said that, can you add a test for this change in test_frame.py ?

Also, the title of your PR says you're changing to_dict(), which could also use a type for orient as well, but I didn't see that in your proposed change.

@KianShah
Copy link
Contributor Author

KianShah commented Jun 23, 2022

I added 'tight' as an acceptable Literal for the to_dict overload. Is that not enough?

@Dr-Irv
Copy link
Collaborator

Dr-Irv commented Jun 24, 2022

We're trying to make it that when we add something to the stubs, we also add a code snippet that tests the stub. So, for example, if you have a small piece of code that fails type checking without your change and succeeds with your change, that would be sufficient.

@Dr-Irv
Copy link
Collaborator

Dr-Irv commented Jul 10, 2022

@KianShah let us know if you intend to complete this PR by adding a test. Thanks.

@KianShah
Copy link
Contributor Author

Yes sorry, I just got really busy. Will update either today or tomorrow

@KianShah
Copy link
Contributor Author

I've updated with some tests for from_dict and to_dict. Not sure how you guys test, but I just put calls to from_dict and to_dict. I also added parameter types in the from_dict method.

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.

Now failing due to formatting issues. Do the following on your local machine, until all tests pass. Then make a new commit and push again, and I will trigger the CI

poetry run poe test_all

tests/test_frame.py Outdated Show resolved Hide resolved
@KianShah
Copy link
Contributor Author

Ok I've run the pre-commit this time and poe test-all returns all green lights

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 @KianShah

@Dr-Irv Dr-Irv merged commit 8fbe101 into pandas-dev:main Jul 19, 2022
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.

None yet

2 participants