Repository: pandas-dev/pandas
Pull request: ENH: Validation to only allow positive integers for options
- [x] closes #23348
- [x] tests added / passed
- [x] passes `black pandas`
- [x] passes `git diff upstream/master -u -- "*.py" | flake8 --diff`
- [x] whatsnew entry

I 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.

Problem statement:
Pandas should validate display.max_rows option
#### Code Sample, a copy-pastable example if possible

```python
import pandas as pd
pd.set_option('display.max_rows', -1)
test = [['a', 'b'] ,['c','d'] ,['f','g']]
pd.DataFrame(test, columns=['columna', 'columnb'])
```
#### Problem description

This incorrectly print's 4 rows:

```
Out[1]:
..     ...     ...
   columna columnb
0        a       b
1        c       d
1        c       d
2        f       g

[3 rows x 2 columns]
```
Pandas should not allow negative numbers for this setting because it does non-sensical things. In many other places -1 is used to represent an unbounded number so setting it -1 is actually intuitive but it doesn't work correctly and we should fail fast instead of confusing users.

#### Expected Output

```
Out[1]:
..     ...     ...
   columna columnb
0        a       b
1        c       d
2        f       g

[3 rows x 2 columns]
```

#### Output of ``pd.show_versions()``

<details>

[paste the output of ``pd.show_versions()`` here below this line]
INSTALLED VERSIONS
------------------
commit: None
python: 3.6.3.candidate.1
python-bits: 64
OS: Linux
OS-release: 4.11.3-67_fbk17_4093_g2bf19e7a0b95
machine: x86_64
processor: x86_64
byteorder: little
LC_ALL: 
LANG: en_US.UTF-8
LOCALE: en_US.UTF-8

pandas: 0.23.4
pytest: None
pip: None
setuptools: 40.4.3
Cython: 0.29a0
numpy: 1.15.1
scipy: 1.1.0
pyarrow: None
xarray: None
IPython: 6.5.0
sphinx: None
patsy: 0.4.1
dateutil: 2.7.3
pytz: 2018.4
blosc: None
bottleneck: None
tables: None
numexpr: None
feather: None
matplotlib: 2.2.3
openpyxl: 2.4.11
xlrd: 0.9.4
xlwt: None
xlsxwriter: None
lxml: 3.5.0
bs4: None
html5lib: 0.9999999
sqlalchemy: 1.1.13
pymysql: None
psycopg2: None
jinja2: 2.10
s3fs: None
fastparquet: None
pandas_gbq: None
pandas_datareader: None
</details>

Hints from issue discussion:
That's a good idea.  We should check all such configuration options and see whether we can perform validation similar to what you are describing for `display.max_rows`.

Commit message: Fixing order as per isort

Patch to review:
diff --git a/doc/source/whatsnew/v0.25.0.rst b/doc/source/whatsnew/v0.25.0.rst
index 68ecb4c487a1e..ade2b281cc2c0 100644
--- a/doc/source/whatsnew/v0.25.0.rst
+++ b/doc/source/whatsnew/v0.25.0.rst
@@ -1188,6 +1188,7 @@ Other
 - Allow :class:`Index` and :class:`RangeIndex` to be passed to numpy ``min`` and ``max`` functions (:issue:`26125`)
 - Use actual class name in repr of empty objects of a ``Series`` subclass (:issue:`27001`).
 - Bug in :class:`DataFrame` where passing an object array of timezone-aware `datetime` objects would incorrectly raise ``ValueError`` (:issue:`13287`)
+- Added a new option validation function (is_pos_int) to allow only positive integers, and set the display.precision, display.max_rows and display.max_columns options to use it (:issue:`23348`)
 
 .. _whatsnew_0.250.contributors:
 
diff --git a/pandas/_config/config.py b/pandas/_config/config.py
index 61e926035c3f2..0d8e7590f8612 100644
--- a/pandas/_config/config.py
+++ b/pandas/_config/config.py
@@ -787,6 +787,7 @@ def is_instance_factory(_type):
                 ValueError if x is not an instance of `_type`
 
     """
+
     if isinstance(_type, (tuple, list)):
         _type = tuple(_type)
         type_repr = "|".join(map(str, _type))
@@ -820,6 +821,34 @@ def inner(x):
     return inner
 
 
+def is_pos_int():
+    """
+    Creates a function for validating that an option value is either a positive
+    integer or a None type
+
+    Returns
+    -------
+    validator - a function of a single argument x , which raises
+                ValueError if type(x) is not equal to `_type` or if x is not
+                positive in the case of a int type.
+
+    """
+
+    _type = (type(None), int)
+    type_repr = "|".join(map(str, _type))
+
+    def inner(x):
+        if not isinstance(x, _type):
+            msg = "Value must be an instance of {type_repr}"
+            raise ValueError(msg.format(type_repr=type_repr))
+        if type(x) == int:
+            if x < 0:
+                msg = "int values must be positive for this option"
+                raise ValueError(msg)
+
+    return inner
+
+
 # common type validators, for convenience
 # usage: register_option(... , validator = is_int)
 is_int = is_type_factory(int)
diff --git a/pandas/core/config_init.py b/pandas/core/config_init.py
index be6086dd360f2..6b956ab6cdf86 100644
--- a/pandas/core/config_init.py
+++ b/pandas/core/config_init.py
@@ -18,6 +18,7 @@
     is_instance_factory,
     is_int,
     is_one_of_factory,
+    is_pos_int,
     is_text,
 )
 
@@ -319,7 +320,7 @@ def is_terminal():
 
 
 with cf.config_prefix("display"):
-    cf.register_option("precision", 6, pc_precision_doc, validator=is_int)
+    cf.register_option("precision", 6, pc_precision_doc, validator=is_pos_int())
     cf.register_option(
         "float_format",
         None,
@@ -333,12 +334,7 @@ def is_terminal():
         pc_max_info_rows_doc,
         validator=is_instance_factory((int, type(None))),
     )
-    cf.register_option(
-        "max_rows",
-        60,
-        pc_max_rows_doc,
-        validator=is_instance_factory([type(None), int]),
-    )
+    cf.register_option("max_rows", 60, pc_max_rows_doc, validator=is_pos_int())
     cf.register_option(
         "min_rows",
         10,
@@ -351,12 +347,7 @@ def is_terminal():
         max_cols = 0  # automatically determine optimal number of columns
     else:
         max_cols = 20  # cannot determine optimal number of columns
-    cf.register_option(
-        "max_columns",
-        max_cols,
-        pc_max_cols_doc,
-        validator=is_instance_factory([type(None), int]),
-    )
+    cf.register_option("max_columns", max_cols, pc_max_cols_doc, validator=is_pos_int())
     cf.register_option(
         "large_repr",
         "truncate",
diff --git a/pandas/tests/config/test_config.py b/pandas/tests/config/test_config.py
index 3f12d1d7a292d..221c6e1613fc6 100644
--- a/pandas/tests/config/test_config.py
+++ b/pandas/tests/config/test_config.py
@@ -208,13 +208,16 @@ def test_set_option_multiple(self):
 
     def test_validation(self):
         self.cf.register_option("a", 1, "doc", validator=self.cf.is_int)
+        self.cf.register_option("d", 1, "doc", validator=self.cf.is_pos_int())
         self.cf.register_option("b.c", "hullo", "doc2", validator=self.cf.is_text)
+
         msg = "Value must have type '<class 'int'>'"
         with pytest.raises(ValueError, match=msg):
             self.cf.register_option("a.b.c.d2", "NO", "doc", validator=self.cf.is_int)
 
         self.cf.set_option("a", 2)  # int is_int
         self.cf.set_option("b.c", "wurld")  # str is_str
+        self.cf.set_option("d", 2)
 
         # None not is_int
         with pytest.raises(ValueError, match=msg):
@@ -222,6 +225,18 @@ def test_validation(self):
         with pytest.raises(ValueError, match=msg):
             self.cf.set_option("a", "ab")
 
+        msg = r"Value must be an instance of <class 'NoneType'>\|<class 'int'>"
+        with pytest.raises(ValueError, match=msg):
+            self.cf.register_option(
+                "a.b.c.d3", "NO", "doc", validator=self.cf.is_pos_int()
+            )
+
+        msg = "int values must be positive for this option"
+        with pytest.raises(ValueError, match=msg):
+            self.cf.register_option(
+                "a.b.c.d3", -2, "doc", validator=self.cf.is_pos_int()
+            )
+
         msg = r"Value must be an instance of <class 'str'>\|<class 'bytes'>"
         with pytest.raises(ValueError, match=msg):
             self.cf.set_option("b.c", 1)


Code review comments:
diff_hunk: @@ -1188,6 +1188,7 @@ Other
 - Allow :class:`Index` and :class:`RangeIndex` to be passed to numpy ``min`` and ``max`` functions (:issue:`26125`)
 - Use actual class name in repr of empty objects of a ``Series`` subclass (:issue:`27001`).
 - Bug in :class:`DataFrame` where passing an object array of timezone-aware `datetime` objects would incorrectly raise ``ValueError`` (:issue:`13287`)
+- Added a new option validation function (is_pos_int) to allow only positive integers, and set the display.precision, display.max_rows and display.max_columns options to use it (:issue:`23348`)
path: doc/source/whatsnew/v0.25.0.rst
text: @user1:
Since the new function is private no need to mention here. Would want to speak to how its visible to an end user, so something like "Trying to set to a non-positive integer will raise..."

@author:
Doc updated with latest commit.

diff_hunk: @@ -820,6 +821,34 @@ def inner(x):
     return inner
 
 
+def is_pos_int():
+    """
+    Creates a function for validating that an option value is either a positive
+    integer or a None type
+
+    Returns
+    -------
+    validator - a function of a single argument x , which raises
+                ValueError if type(x) is not equal to `_type` or if x is not
+                positive in the case of a int type.
+
+    """
+
+    _type = (type(None), int)
+    type_repr = "|".join(map(str, _type))
+
+    def inner(x):
+        if not isinstance(x, _type):
path: pandas/_config/config.py
text: @user1:
I think you could simplify this if you just used a condition like `if not (x is None or isinstance(x, int))`

@author:
Agreed,  since the simplified function isn't really dynamic, the latest commit modified is to just be called standalone.

diff_hunk: @@ -319,7 +320,7 @@ def is_terminal():
 
 
 with cf.config_prefix("display"):
-    cf.register_option("precision", 6, pc_precision_doc, validator=is_int)
+    cf.register_option("precision", 6, pc_precision_doc, validator=is_pos_int())
path: pandas/core/config_init.py
text: @user1:
Hmm I don't think this is correct. Isn't this evaluating `is_pos_int()` and returning None, so essentially doing `validator=None`?

@author:
This was the correct way to call it, but i didn't really like it as it looked different from the calls to is_int and is_text.  Fixed with the latest commit.