REF: add custom Exception for safe_sort #25569
Conversation
Codecov Report
@@ Coverage Diff @@
## master #25569 +/- ##
==========================================
- Coverage 91.77% 91.76% -0.02%
==========================================
Files 175 175
Lines 52607 52624 +17
==========================================
+ Hits 48282 48288 +6
- Misses 4325 4336 +11
Continue to review full report at Codecov.
|
pandas/errors/__init__.py
Outdated
| @@ -157,6 +157,13 @@ class MergeError(ValueError): | |||
| """ | |||
|
|
|||
|
|
|||
| class SortError(TypeError): | |||
| """ | |||
| Error raised when problems arise during sorting due to problems | |||
There was a problem hiding this comment.
Is it possible to be more explicit about the types of "problems" that could arise? Is this just when sorting mixed types?
There was a problem hiding this comment.
@WillAyd : the rationale here was to a create an exception analogous to the custom MergeError. Exceptions should be considered part of the api and so this SortError is a subclass of TypeError to be backwards compatible with the exceptions currently raised by np.sort from safe_sort and therefore result in a non-breaking api change.
Rather than be explicit on the exceptions covered, I consider this to be a base class for any sort related errors that may need to be handled. (although i would probably not have chosen TypeError as the base class). I should probably subclass this again for the safe_sort errors, but that is not strictly necessary at the moment.
|
what is the reason for this change? |
during the process of converting the pytest.raises tests to use a context manager, two tests have so far had the match parameter added to check that the correct error is raised, but these were only testing safe_sort and factorise helper functions directly. There are still many tests that require the match parameter added and there are several instances in the code base where the calls to safe-sort are not wrapped in a try/except block. This PR is not critical at the moment and can be put on hold until more tests have been updated and further work on testing error messages is completed. |
|
@simonjayhawkins right not objecting to making testing easier. This exception is a good idea, though have to think of implications for a user level exception like this. |
i'm assuming that will need to wrap all uses of safe_sort and then raise an appropriate user facing Exception or RunTimeWarning (I believe that @reidy-p has already started doing this). so the SortError is not intended to be user facing. |
yep exactly (though maybe we just make it an internal exception for this reason) |
do you mean define the Exception class in |
|
actually I kind of like this custom exception now. |
|
I am personally not a fan of adding custom user-level exceptions if there is no good reason for it (and here it only seems for testing?) But is it the intent to expose it to users? The discussion above about it is not fully clear to me. |
|
if you can merge master and update. I believe this would be ok if SortError is just an internal exception (e.g. define in pandas/core/sorting), mainly to disambiguate on testing; raising an appropriate error to users. |
|
Note that the current version of the PR does expose it publicly (from a quick look at the diff in |
right my suggestion was to make it internal |
|
started to internalize the custom error, still need to wrap other calls to safe_sort. |
|
i'm not sure that at the moment there is much to gain from the added complexity here. and also there is the helper function regexes should probably just be simplified to |
|
@simonjayhawkins agree with your comment. |
xref #25537 (comment)
custom
Exceptionadded primarily to allow more targeted testing, but also to distinguish sort failures and bad input tosafe_sort