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

GH1043 Update _typing.pyi: add "cross" option to JoinHow #1044

Merged
merged 6 commits into from
Nov 26, 2024

Conversation

brzegorzTesco
Copy link
Contributor

I haven't added tests since JoinHow doesn't seem to be tested in any case. Should I add some? Merge tests unimpacted.

@brzegorzTesco brzegorzTesco changed the title Update _typing.pyi: add "cross" option to JoinHow GH1043 Update _typing.pyi: add "cross" option to JoinHow Nov 21, 2024
Comment on lines 732 to 733
JoinHow: TypeAlias = Literal["left", "right", "outer", "inner", "cross"]
MergeHow: TypeAlias = JoinHow
Copy link
Collaborator

Choose a reason for hiding this comment

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

This fix is not correct, because JoinHow is used as the type of the join argument in the align() method.

The right fix is to change the typing of DataFrame.join() to use the existing MergeHow type.

You would then add a test like your example that shows that "cross" is accepted as a value for the how argument of DataFrame.join()

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Thank you for the quick feedback, I've done as you said.

Comment on lines 2351 to 2355
df1.join(df2, how="cross")
df1.join(df2, how="inner")
df1.join(df2, how="outer")
df1.join(df2, how="left")
df1.join(df2, how="right")
Copy link
Collaborator

Choose a reason for hiding this comment

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

Can you use the check(assert_type(... pattern here?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Done

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.

CI is failing because you have to fix formatting. See https://github.com/pandas-dev/pandas-stubs/actions/runs/12008445166/job/33489766438?pr=1044

Best to create a dev environment via the instructions here so that pre-commit will pick up things like this:
https://github.com/pandas-dev/pandas-stubs/blob/main/docs/setup.md

Fix formatting
@brzegorzTesco
Copy link
Contributor Author

Thank you for the link, I haven't found it on my own previously. I've set up an dev environment, this commit should pass pre-commit checks.

@Dr-Irv Dr-Irv merged commit 4ac23d6 into pandas-dev:main Nov 26, 2024
10 checks passed
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.

loicdiridollou pushed a commit to loicdiridollou/pandas-stubs that referenced this pull request Nov 27, 2024
…1044)

* Update _typing.pyi: add "cross" option to JoinHow

* Removed "cross" option from JoinHow and added it back to MergeHow

* Replaced JoinHow with MergeHow in frame.join

* Added a test for join "how"

* Update test_frame.py: used check(assert_type pattern in unit test

* Update test_frame.py

Fix formatting
Dr-Irv added a commit that referenced this pull request Nov 27, 2024
* GH1051 Drop astype tests for Mac arm for float128 and complex256

* Fix

* Fix

* Test fix

* Move to macos-latest

* Update test.yml

* GH1051 Lock in tables version to 3.10.1

* GH1049 Deprecate Index and Grouper methods for 2.0 migration (#1050)

* GH1049 Deprecate Index and Grouper methods for migration to 2.0

* GH1049 Deprecate use_nullable_dtypes in read_parquet

* Change signature of StringMethods.rsplit to match pandas (#1056)

* GH1053 @ for DataFrame, eval in place for DataFrame, test migrations (#1054)

* GH1053 @ for DataFrame, eval in place for DataFrame, test migrations

* Formatting and spelling

* GH1053 PR Feedback

* GH1053 Formatting

* GH1043 Update _typing.pyi: add "cross" option to JoinHow (#1044)

* Update _typing.pyi: add "cross" option to JoinHow

* Removed "cross" option from JoinHow and added it back to MergeHow

* Replaced JoinHow with MergeHow in frame.join

* Added a test for join "how"

* Update test_frame.py: used check(assert_type pattern in unit test

* Update test_frame.py

Fix formatting

* Version 2.2.3.241126

---------

Co-authored-by: kroche98 <[email protected]>
Co-authored-by: brzegorzTesco <[email protected]>
Co-authored-by: Irv Lustig <[email protected]>
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.

[mypy] Dataframe.Join with "cross" option for "how" argument throws error even though it's valid
2 participants