DOC/ERR: better error message on no common merge keys by swyoon · Pull Request #19427 · pandas-dev/pandas (original) (raw)
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 andprivacy statement. We’ll occasionally send you account related emails.
Already on GitHub?Sign in to your account
Conversation14 Commits1 Checks0 Files changed
Conversation
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.Learn more about bidirectional Unicode characters
[ Show hidden characters]({{ revealButtonHref }})
- [o] closes DOC/ERR: better error message on no common merge keys #19391
- [o] tests passed
- [o] passes
git diff upstream/master -u -- "*.py" | flake8 --diff - Let MergeError emit values keyword arguments
- Add
DataFrame.joinonSee alsosection ofpandas.merge
It seems that the failed test has nothing to do with the content of the commit though..
Can you add a test for this, and a release note?
What's the output look like for large DataFrames? Does the repr truncation work correctly?
| @@ -233,7 +233,7 @@ |
|---|
| -------- |
| merge_ordered |
| merge_asof |
| DataFrame.join : For index-based merge operations |
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think you can remove the explanation, everything other than the name. I'm not sure what numpydoc does with it, but those are turned into hyperlinks for the HTML docs.
Release note added, see also modified.
I believe my contribution is about the error message. Should the error message be included in the test?
It seems that there already exists a test for a merge with no common keys.
I don't get what you said about the large DataFrames. Could you explain a bit more?
I believe my contribution is about the error message. Should the error message be included in the test?
Yes, see pandas.util.testing.assert_raises_regex for how to do it. You'll want to do a few different tests and assert that the correct information is always in the exception message.
Ignore my comment about large dataframes I think.
| @@ -1028,7 +1028,14 @@ def _validate_specification(self): |
|---|
| common_cols = self.left.columns.intersection( |
| self.right.columns) |
| if len(common_cols) == 0: |
| raise MergeError('No common columns to perform merge on') |
| raise MergeError('No common columns to perform merge on. ' |
| 'Merge options: right_on={ron}, ' |
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Can you change the order to be left_on, right_on, left_index, right_index, to match the order in the signature?
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Oh sure I can. will do it.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
to make this more readable you can break it like
raise MergeError(
'No common ......'
)
| @@ -1028,7 +1028,14 @@ def _validate_specification(self): |
|---|
| common_cols = self.left.columns.intersection( |
| self.right.columns) |
| if len(common_cols) == 0: |
| raise MergeError('No common columns to perform merge on') |
| raise MergeError('No common columns to perform merge on. ' |
| 'Merge options: right_on={ron}, ' |
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
to make this more readable you can break it like
raise MergeError(
'No common ......'
)
| 'left_on={lon}, right_index={ridx}, ' |
|---|
| 'left_index={lidx}' |
| .format(ron=self.right_on, |
| lon=self.left_on, |
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
can you update the test for this (it prob just raises) and match the error message with assert_raises_regex
can you rebase and update
The line break and the order of options are modified.
Added a check for error message by assert_raises_regex.
- Let MergeError emit values keyword arguments.
- Add DataFrame.join on 'See also' section of pandas.merge.
- Add an item in whatsnew
- Update test_no_overlap_more_informative_error to check the message from MergeError
harisbal pushed a commit to harisbal/pandas that referenced this pull request
