Skip to content

Conversation

@ramvikrams
Copy link
Contributor

I added the parameters, but just a question do we have to add test in these compatibility changes also

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.

Fix as suggested, and investigate failures in mypy

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.

I think, but I'm not sure, that the changes I suggested will fix your problems. You have to really understand how typing works to debug what is going on. Make the changes incrementally, and test locally.

@ramvikrams
Copy link
Contributor Author

I think, but I'm not sure, that the changes I suggested will fix your problems. You have to really understand how typing works to debug what is going on. Make the changes incrementally, and test locally.

Yes now I'll try harder in the all upcoming issues

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.

Can you also add a set of tests for this, i.e., test index=True and index=False with different values of orient . Test the index arguments of add_prefix() and add_suffix() as well.

When index=False is not allowed, then just do an assert_type() test, surrounded by if TYPE_CHECKING_INVALID_USAGE: with a correct # type: ignore[???] # pyright: ignore[reportGeneralTypeIssues] Idea here is to make sure that from a typing perspective, we have tested that we are rejecting False when it is not allowed.

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 5dd1820 into pandas-dev:main Apr 12, 2023
@ramvikrams ramvikrams deleted the t130 branch April 12, 2023 16:58
@ramvikrams
Copy link
Contributor Author

thanks @ramvikrams

Thanks sir

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