ENH: Validation to only allow positive integers for options #27382
Conversation
set up so that they can be either a None type or a positive int. Updated the option creation code for display options that failed when set to negative numbers.
returning an additional function. This also allows it to be used in an identical fasion to is_int, is_text etc. Modified the validation calls to is_pos_int to remove (). Modified the tests using is_pos_int to remove (). Updated whatsnew documentation to be more user focused.
pandas/_config/config.py
Outdated
|
|
||
| Parameters | ||
| ---------- | ||
| value - the value to be checked |
There was a problem hiding this comment.
@jreback comment for returns section is lost in diff so re-commenting here but I'm surprised this isn't failing the CI either
@datapythonista do we ignore private docstrings in the validation? Vaguely recall some comment around that but didn't see code to confirm
There was a problem hiding this comment.
Sorry, shall I add the following?
Returns
----------
Nothing
There was a problem hiding this comment.
@WillAyd yes, we don't validate docstrings of private functions. Ideally we should validate everything, but with all the pending work in the public API that has an impact in the web docs, I don't think we should spend time in fixing the private docstrings yet (and if we don't fix the existing cases, we can't easily validate the new ones).
There was a problem hiding this comment.
Thanks for the clarification - do you know where that exclusion happens? Didn't see it in code_checks and couldn't find on quick glance in validate_docstrings.py
There was a problem hiding this comment.
Ups sorry, was going to mention and I forgot. It's not that we exclude them, it's that we get the docstrings to consider from the .rst files of the API docs. The code here is a good starting point to understand that: https://github.com/pandas-dev/pandas/blob/master/scripts/validate_docstrings.py#L873
There was a problem hiding this comment.
@Adam-Klaum these need to be updated to follow the doctoring stander. See https://dev.pandas.io/development/contributing.html#updating-a-pandas-docstring
|
py37_np_dev test is failing with the error below. Not sure why. I notice this is happening on other pull requests as well: |
|
Is there anything else I need to do on this one? It still shows as Changes requested, but those changes have been integrated into later commits. I just want to make sure nobody is waiting on me for this one. |
as the Raises keyword in docstring.
doc/source/whatsnew/v1.0.0.rst
Outdated
|
|
||
| Other | ||
| ^^^^^ | ||
| - Trying to set the display.precision, display.max_rows or display.max_columns using set_option to anything but a None or a positive int will raise a ``ValueError`` (:issue:`23348`) |
There was a problem hiding this comment.
Put the option names like display.precision in double-backticks.
set_option -> :func:`pandas.set_option`
a None -> None
int -> integer.
pandas/_config/config.py
Outdated
|
|
||
| Parameters | ||
| ---------- | ||
| value - the value to be checked |
There was a problem hiding this comment.
@Adam-Klaum these need to be updated to follow the doctoring stander. See https://dev.pandas.io/development/contributing.html#updating-a-pandas-docstring
pandas/_config/config.py
Outdated
|
|
||
| """ | ||
|
|
||
| if not (value is None or isinstance(value, int)): |
There was a problem hiding this comment.
Use isistance(values, numbers.Integral)
Can you restructure this a bit?
if value is None:
return value
elif isinstance(value, numbers.Integral):
if value < 0:
raise
return value
raise ValueError(...)
pandas/_config/config.py
Outdated
| """ | ||
|
|
||
| if not (value is None or isinstance(value, int)): | ||
| msg = "Value must be an instance of <class 'NoneType'>|<class 'int'>" |
There was a problem hiding this comment.
I think most people won't be familiar with NoneType. Should probably say like 'value' must be a positive integer or None.
Simplified is_pos_int function and updated tests accordingly Rewrote is_pos_int docstring to conform to standards
pandas/_config/config.py
Outdated
|
|
||
| Raises | ||
| ------ | ||
| ValueError if the value is not equal to a positive integer, 0 or None. |
There was a problem hiding this comment.
I think this should be formatted like
Raises
-------
ValueError
When the value is not a positive integer or None
There was a problem hiding this comment.
Yes, 0 is allowed, and is necessary for existing internal code. I think you are right, at this point "is_pos_int" is technically not an accurate name. I'm fine with "is_nonnegative_int". The docstring change seems fine to me as well. I'll make the changes and do another push.
doc/source/whatsnew/v1.0.0.rst
Outdated
|
|
||
| Other | ||
| ^^^^^ | ||
| - Trying to set the ``display.precision``, ``display.max_rows`` or ``display.max_columns`` using set_option to anything but a None or a positive int will raise a ``ValueError`` (:issue:`23348`) |
There was a problem hiding this comment.
use double backticks around None. use :meth:`set_option`
|
lgtm. over to you @WillAyd |
|
thanks @Adam-Klaum ! |
black pandasgit diff upstream/master -u -- "*.py" | flake8 --diffI looked at several different options for providing this functionality. In the end it seemed best to create a a new function that returns a validation function to only allow positive integers. So now we have is_pos_int available along side the existing is_int and is_text function. display.max_rows has been modified to use this new validation. I went through the other integer based display options and tested each with a negative int to see what else might be broken. display.precision and display.max_columns both had issues when set to a negative number prior to displaying a dataframe. They have been changed as well.