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

Raise more specific exception on concat([None]) #8501

Closed
wants to merge 2 commits into from

Conversation

jason-curtis
Copy link
Contributor

More specific exception allows for a safer, more explicit catch by a caller.
ConcatenationError follows the pattern of MergeError which is used in the same file when something is wrong with a caller's inputs to merge().

@jreback
Copy link
Contributor

jreback commented Oct 7, 2014

@jorisvandenbossche you think this is an API change? seems completely back-compat

@jreback jreback added the Error Reporting Incorrect or improved errors from pandas label Oct 7, 2014
@@ -679,7 +683,7 @@ def concat(objs, axis=0, join='outer', join_axes=None, ignore_index=False,
If a dict is passed, the sorted keys will be used as the `keys`
argument, unless it is passed, in which case the values will be
selected (see below). Any None objects will be dropped silently unless
they are all None in which case an Exception will be raised
they are all None in which case a ConcatenationError will be raised
Copy link
Member

Choose a reason for hiding this comment

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

Maybe this could just be a ValueError?

I'm not opposed to more specific exceptions, but if you are going to define a new exception, it probably should be exposed a higher level of the API than pandas.tools.merge. I'm guessing there is precedent for this somewhere?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

The pattern I'm following is from MergeError (declared on line 44). MergeError is raised when something is wrong with a caller's inputs to merge().

ValueError would also be an improvement but I figured I should follow the pattern used in the file.

Copy link
Member

Choose a reason for hiding this comment

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

OK, fair enough.

@jorisvandenbossche
Copy link
Member

Yeah, as far as I understand it correctly, this should be back compat (as if a user is checking for an Exception now, this will still work for ConcatenationError).

However, I do follow @shoyer here in the question if it is not more useful to provide eg ValueError.
I know it would be more consistent with the rest of the file to use a custom error, but is that pattern used a lot elsewhere in pandas? (@jreback?) If we think something like ValueError is better, I think we should do that regardless of consistency with merge.
And I personally find ValueError possibly more informative than ConcatenationError, as there are more things that could go wrong in the concatenation function than wrong input values.

@jreback
Copy link
Contributor

jreback commented Oct 8, 2014

I think MergeError could be changed to ValueError. Which is what this really is, either the arguments are incorrect (e.g. can't specify multiple conflicting options), or your data is not-unique.

So IMHO, I think let's make MergeError inherit from ValueError (I think this is back-compat), but put in a note that we are getting rid of it in 0.16? (or just drop it)?
and make this bare Exception into a ValueError

@jason-curtis
Copy link
Contributor Author

Updated to remove ConcatenationError and make ValueError more central, as suggested. I'll leave the deprecation of MergeError to you folks if that's OK. Not sure how you'd like to document/go forward with that kind of thing.

@jorisvandenbossche
Copy link
Member

Can you sqaush your commits?

@jason-curtis
Copy link
Contributor Author

@jorisvandenbossche done!

@jreback
Copy link
Contributor

jreback commented Oct 9, 2014

@thatneat can you add a release note to v0.15.0 API changes (reference this issue, saying a MergeError will now specialize as a ValueError or something like that).

squash and I think good to go

@jreback jreback added this to the 0.15.0 milestone Oct 9, 2014
@jreback
Copy link
Contributor

jreback commented Oct 10, 2014

merged via ce79c80

thanks!

@jreback jreback closed this Oct 10, 2014
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Error Reporting Incorrect or improved errors from pandas
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

5 participants