Skip to content

ENH: Improve typing of some general functions #355

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.

Sign up for GitHub

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

Jump to bottom
Merged
merged 24 commits into from
Oct 7, 2022
Merged

ENH: Improve typing of some general functions #355

merged 24 commits into from
Oct 7, 2022

Conversation

bashtage
Copy link
Contributor

@bashtage bashtage commented Oct 5, 2022

  • Tests added: Please use assert_type() to assert the type of any return value

Dr-Irv
Dr-Irv requested changes Oct 6, 2022
View reviewed changes
Dr-Irv
Dr-Irv requested changes Oct 6, 2022
View reviewed changes
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.

You added typing for pd.cut() and pd.qcut(), but no tests. Do you want to add them here, or maybe handle cut and qcut in a separate PR?

Dr-Irv
Dr-Irv reviewed Oct 6, 2022
View reviewed changes
Dr-Irv
Dr-Irv reviewed Oct 6, 2022
View reviewed changes
@bashtage bashtage mentioned this pull request Oct 7, 2022
Merged
1 task
@bashtage
Copy link
Contributor Author

bashtage commented Oct 7, 2022

@Dr-Irv should be ready for final review

Dr-Irv
Dr-Irv requested changes Oct 7, 2022
View reviewed changes
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.

still some small things to clean up

@Dr-Irv Dr-Irv mentioned this pull request Oct 7, 2022
Closed
2 tasks
Dr-Irv
Dr-Irv reviewed Oct 7, 2022
View reviewed changes
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.

Code looks OK. ping when green.

@bashtage
Copy link
Contributor Author

bashtage commented Oct 7, 2022

@Dr-Irv it is green now.

Dr-Irv
Dr-Irv requested changes Oct 7, 2022
View reviewed changes
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.

Just remembered one more thing based on #289 which hasn't been updated for a while.

pandas-stubs/core/reshape/merge.pyi Outdated
on: Label | Sequence | AnyArrayLike | None = ...,
left_on: Label | Sequence | AnyArrayLike | None = ...,
right_on: Label | Sequence | AnyArrayLike | None = ...,
how: Literal["left", "right", "outer", "inner", "cross"] = ...,
Copy link
Collaborator

Choose a reason for hiding this comment

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

Ah, just remembered this. We have MergeHow in _typing.pyi, which is missing "cross", so can you pull in the test from #289 and then I will close that, and update the how argument here to use MergeHow. Then we can also close issue #288 .

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Fixed.

Dr-Irv
Dr-Irv approved these changes Oct 7, 2022
View reviewed changes
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 @bashtage for your patience in getting all the changes in!

@Dr-Irv Dr-Irv merged commit d2ae5ee into pandas-dev:main Oct 7, 2022
@Dr-Irv Dr-Irv mentioned this pull request Oct 7, 2022
Closed
@bashtage bashtage deleted the gen-funcs-v2 branch October 8, 2022 06:51
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@Dr-Irv Dr-Irv Dr-Irv approved these changes

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
2 participants
@bashtage @Dr-Irv