Skip to content

Conversation

ahmedibrhm
Copy link
Contributor

@ahmedibrhm ahmedibrhm commented Jul 8, 2022

This is following up on the pull request #44365

@pep8speaks
Copy link

pep8speaks commented Jul 8, 2022

Hello @ahmedibrhm! Thanks for updating this PR. We checked the lines you've touched for PEP 8 issues, and found:

There are currently no PEP 8 issues detected in this Pull Request. Cheers! 🍻

Comment last updated at 2022-07-16 04:17:06 UTC

Copy link
Member

@phofl phofl 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 add tests for invalid arguments? Wrong length, not a tuple, ...

@jreback jreback added Reshaping Concat, Merge/Join, Stack/Unstack, Explode Enhancement labels Jul 8, 2022
@jreback jreback added this to the 1.5 milestone Jul 8, 2022
@@ -75,6 +75,11 @@
keep_equal : bool, default False
If true, the result keeps values that are equal.
Otherwise, equal values are shown as NaNs.

suffixes : tuple, default ('self', 'other')
Copy link
Contributor

Choose a reason for hiding this comment

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

i think we should call this names or maybe result_names as suffixes implies we are appending these to something (ala merge)

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@jreback Sorry, I didn't see the comment.
I guess suffix should be used because to some extent the result will be merged. What I mean is that the comparison will be one block representing the different parts of the two dataframes.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I can see your point but result names will mean that the result will have names. like we have an output which will have multiple names

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@jreback I edited the name of the argument

@ahmedibrhm ahmedibrhm requested a review from phofl July 9, 2022 01:27
@phofl
Copy link
Member

phofl commented Jul 9, 2022

small comments, docs fail

@ahmedibrhm ahmedibrhm requested a review from phofl July 10, 2022 18:32
Copy link
Contributor

@jreback jreback left a comment

Choose a reason for hiding this comment

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

let's discuss the name of this added arg

@ahmedibrhm ahmedibrhm requested a review from jreback July 11, 2022 14:14
@ahmedibrhm ahmedibrhm changed the title ENH: add suffixes argument to DataFrame.compare #44354 ENH: add result_names argument to DataFrame.compare #44354 Jul 12, 2022
@ahmedibrhm ahmedibrhm marked this pull request as draft July 12, 2022 20:17
@ahmedibrhm ahmedibrhm marked this pull request as ready for review July 13, 2022 16:07
Copy link
Contributor

@jreback jreback left a comment

Choose a reason for hiding this comment

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

lgtm

@jreback
Copy link
Contributor

jreback commented Jul 15, 2022

@phofl if any comments

Copy link
Member

@phofl phofl left a comment

Choose a reason for hiding this comment

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

Please add a test that raises because of an invalid input

@ahmedibrhm ahmedibrhm requested a review from phofl July 16, 2022 05:32
@ahmedibrhm
Copy link
Contributor Author

@phofl done, added the test

@phofl phofl merged commit cb5a924 into pandas-dev:main Jul 16, 2022
@phofl
Copy link
Member

phofl commented Jul 16, 2022

Thx @ahmedibrhm

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Enhancement Reshaping Concat, Merge/Join, Stack/Unstack, Explode
Projects
None yet
Development

Successfully merging this pull request may close these issues.

ENH: add suffixes argument to DataFrame.compare
4 participants