BUG: raise when sort_index with ascending=None #39451
Conversation
ivanovmg
force-pushed
the
sort_values
branch
3 times, most recently
from
971b3a9 to
7717f8b
Compare
pandas/util/_validators.py
Outdated
| Value to be validated. | ||
| arg_name : str | ||
| Name of the argument. To be reflected in the error message. | ||
| none_allowed : bool, optional |
pandas/util/_validators.py
Outdated
| Name of the argument. To be reflected in the error message. | ||
| none_allowed : bool, optional | ||
| Whether to consider None to be a valid boolean. | ||
| int_allowed : bool, optional |
pandas/core/generic.py
Outdated
| @@ -11772,3 +11774,12 @@ def _doc_params(cls): | |||
| The required number of valid values to perform the operation. If fewer than | |||
| ``min_count`` non-NA values are present the result will be NA. | |||
| """ | |||
|
|
|||
|
|
|||
| def validate_ascending(ascending): | |||
There was a problem hiding this comment.
can you move to pandas/util/validators.py
shouldn't this raise for a list-like case if something is wrong?
There was a problem hiding this comment.
e.g. this shouldn't return anything
pls add some typing
There was a problem hiding this comment.
It raises for a list-like case if any of the items in ascending list is not treated as a valid boolean.
This construct does just that.
return [validate_bool_kwarg(item, "ascending", **kwargs) for item in ascending]
There was a problem hiding this comment.
this can simply be part of validate_bool_kwarg or another function but should be co-located with validate_bool_kwarg
There was a problem hiding this comment.
Moved the function to pandas/util/_validators.py.
It is not clear for me at the moment how to make this validation anyhow different (as a part of validate_bool_kwarg) due to the fact that multiple types are acceptable for ascending.
pandas/core/generic.py
Outdated
| @@ -11772,3 +11774,12 @@ def _doc_params(cls): | |||
| The required number of valid values to perform the operation. If fewer than | |||
| ``min_count`` non-NA values are present the result will be NA. | |||
| """ | |||
|
|
|||
|
|
|||
| def validate_ascending(ascending): | |||
There was a problem hiding this comment.
this can simply be part of validate_bool_kwarg or another function but should be co-located with validate_bool_kwarg
|
can you merge master and fixup to comments. |
| (True, None), | ||
| (False, "True"), |
There was a problem hiding this comment.
can you make these lists
There was a problem hiding this comment.
and maybe add an empty list too
There was a problem hiding this comment.
Currently an empty list does not raise an error, so the test fails.
Should I change the code, so that an empty list raises?
ivanovmg
force-pushed
the
sort_values
branch
from
6559b4c to
860c8a5
Compare
|
ok looks fine. can you add a whtasnew note for 1.2.3 in regression section. merge master and ping on green. |
|
thanks @ivanovmg |
|
@meeseeksdev backport 1.2.x |
This comment has been minimized.
This comment has been minimized.
…40083) Co-authored-by: Maxim Ivanov <[email protected]>
Added validation of
ascendingforsort_indexmethods.Proposed a solution to raise
ValueErrorwhen either of the elements ofascendingturned out to beNone.