Skip to content

TYP : DataFrame.(merge, join) core.reshape.merge.(merge, ...) (easy) #38468

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

Merged
merged 10 commits into from
Dec 16, 2020

Conversation

arw2019
Copy link
Member

@arw2019 arw2019 commented Dec 14, 2020

  • closes #xxxx
  • tests added / passed
  • passes black pandas
  • passes git diff upstream/master -u -- "*.py" | flake8 --diff
  • whatsnew entry

@jreback jreback added the Typing type annotations, mypy/pyright type checking label Dec 14, 2020
@jreback jreback added this to the 1.3 milestone Dec 14, 2020
@jreback
Copy link
Contributor

jreback commented Dec 14, 2020

looks good. @simonjayhawkins if you can double check here.

Copy link
Member

@simonjayhawkins simonjayhawkins left a comment

Choose a reason for hiding this comment

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

Thanks @arw2019 lgtm

in _join_compat, on is passed onto left_on in core.reshape.merge.

presumably both left_on and right_on can also be typed as Optional[IndexLabel] (in DataFrame.merge, core.reshape.merge and _MergeOperation)

left_index: bool = False,
right_index: bool = False,
sort: bool = False,
suffixes: Tuple[str, str] = ("_x", "_y"),
Copy link
Member

Choose a reason for hiding this comment

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

FYI. suffixes can currently be list-like but will change in the future. but I think OK to restrict to tuple here.

Copy link
Member Author

Choose a reason for hiding this comment

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

Is it worth creating a Suffix alias? (So there's a single place where the behavior can be changed?) I would also change this to List if doing that

Copy link
Member

Choose a reason for hiding this comment

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

yes, probably worth creating an alias as used in several places.

I don't think we need to include Sequence though (situation maybe different if types were public/not experimental or we were using a stub that had support for multiple pandas versions).

@arw2019 arw2019 changed the title TYP : DataFrame.merge, core.reshape.merge.(merge, _MergeOperation) (easy) TYP : DataFrame.(merge, join) core.reshape.merge.(merge, _MergeOperation, ...) (easy) Dec 15, 2020
@arw2019 arw2019 changed the title TYP : DataFrame.(merge, join) core.reshape.merge.(merge, _MergeOperation, ...) (easy) TYP : DataFrame.(merge, join) core.reshape.merge.(merge, ...) (easy) Dec 15, 2020
@jreback jreback merged commit 8455f57 into pandas-dev:master Dec 16, 2020
@jreback
Copy link
Contributor

jreback commented Dec 16, 2020

thanks @arw2019

@arw2019 arw2019 deleted the typing-merge branch December 16, 2020 01:09
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Typing type annotations, mypy/pyright type checking
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants