Repository: pytest-dev/pytest
Pull request: Sanitize ini-options default handling #11282
**Overview**
This PR fixes #11282. As per the issue, if an option is created with default value as None and a value for the option is not set in the ini file, then trying to get the option value in code with config.getini() returns an empty list [] instead of None as expected.

**Analysis**
On investigating it was found that the logic to return a default value for a configuration option is as follows:
if a default value is set during the creation of the option, then the set default value is returned
if a default value is NOT set during the option creation but the type for the option is provided, then return an empty list
if neither a default value nor type is provided while creating the option, then assume its a string type and return an empty string

The above logic did not seem right. Also, the logic above did not honor None as an explicit default value to return.

**Solution**
The solution provided in this PR will change the behavior in the following way:
if a default value is NOT set during the option creation but the type for the option is provided, then a type specific default will be returned. That is as follows:
	"paths", "pathlist", "args", "linelist" types will have a default empty list [] as these are all lists
	"bool" will have False as default
	"string" will have empty string "" as default
	logically there can't be any other type as the code checks for valid types before hand. However, just as a fail safe for future,        any other types will have a default of None

if a default value is set explicitly as **None** during the option creation, then **None** will be returned as the default value irrespective of the type
if neither a default value nor type is provided while creating the option, assume its a string type and return an empty string. (This assumption stays as it is from the existing code)

**Checklist**
- [ ] Include documentation when adding new features.
- [X] Include new tests or update existing tests when applicable.
- [X] Allow maintainers to push and squash when merging my commits. Please uncheck this if you prefer to squash the commits yourself.

Problem statement:
config.getini returns an empty list for an option of type string absent in INI file
I want to manage this INI option 
```
    parser.addini(
        "my_option",
        type="string",
        default=None,
        help="My option",
    )
```

I decide not to include this option in the INI file

I read the value with this code

```
@pytest.fixture(scope='session')
def my_option(request):
    return request.config.getini("my_option")
```

Actual result: The returned value is an empty list

Expected result: None

Tested with pytest 7.4.0 on Linux Mint

Hints from issue discussion:
Here is an actual reproducer:

`conftest.py`:

```python
import pytest

def pytest_addoption(parser):
    parser.addini(
        "my_option",
        type="string",
        default=None,
        help="My option",
    )

@pytest.fixture(scope='session')
def my_option(request):
    return request.config.getini("my_option")
```

`test_x.py`:

```python
def test_x(my_option):
    assert my_option is None
```

results in:

```python
    def test_x(my_option):
>       assert my_option is None
E       assert [] is None
```
Hello @harmin-parra, @The-Compiler, @Zac-HD,

I can work on this fix if you all agree.

Regards
Sharad
Hello all,

I have a question regarding this issue. Should **config.getini()** return "None" irrespective of the "type" provided for the configuration? What if the the "type" is linelist or pathlist with no default value or "None" provided in configuration or while making the call to **parser.addini()**

Please look at a test that exists in the current code base that implicitly assumes that the configuration item named "xy" has a default value of empty list. I think the correct way to call **parser.addini()** is to always provide a default value if you are expecting it to return an Empty List or any other data type. Which means the call to addini() in the test code below should be:

parser.addini("xy", "", type="linelist", **default=[]**)
instead of just
parser.addini("xy", "", type="linelist")

```
 def test_addinivalue_line_new(self, pytester: Pytester) -> None:
        pytester.makeconftest(
            """
            def pytest_addoption(parser):
                parser.addini("xy", "", type="linelist")
        """
        )
        config = pytester.parseconfig()
        assert not config.getini("xy")
        config.addinivalue_line("xy", "456")
        values = config.getini("xy")
        assert len(values) == 1
        assert values == ["456"]
        config.addinivalue_line("xy", "123")
        values = config.getini("xy")
        assert len(values) == 2
        assert values == ["456", "123"]
```

Please let me know your views as there are some occurrences within the code base that makes this assumption of implicit default values based on "type".

Regards
Sharad
the issue lies in https://github.com/pytest-dev/pytest/blob/b73b4c464c6fa54f55a1c6ed8eaceb5529a161bb/src/_pytest/config/__init__.py#L1521-L1528

it tries to be smart based on type, but does not quite deal with all kinds of types correctly

as far as i can tell there also is the case of automatically returning "" on a empty default for a string when no type was defined (none implies string)

maybe using something like dataclasses.Missing as a experiment to see how to make sane default return values could be a starting point





Thank you @RonnyPfannschmidt,

But my question was around the expected behavior after making changes for this issue. This issue as raised by @harmin-parra expects config.getini() to return "None" if "None" is set as the default value while adding the configuration item using parser.addini().

**Solution 1**
Return "None" as the default value if "None" or no default value if provided by the developer. In this approach, existing calls to the config.getini() function would need to check for "None" values

**Solution 2**
Return a default value based on the "type" argument. If no type argument is specified, then it will be treated as a string. But in this approach, "None" will never be returned as a default value.

**Solution 3**
The default value can have an additional possible option of "NotSet" which would mean that the developer did not pass any value for the "default" parameter. This will be different from passing "None" as default. In this approach, the following behavior will be seen:

1) If the developer sets "None" as the default value for the configuration, then "None" will be returned irrespective of the "type" parameter.
2) if the developer does not provide any value for the configuration, then it will be treated as "NotSet" and an appropriate default value based on the "type" argument will be returned. If no "type" argument is provided, the an empty string will be returned.

Please let me know which approach would be more acceptable?

Regards
Sharad

I personally feel, Solution 1 is the right way to go considering the straight forward and clean approach.

regards
Sharad
+1 for solution 1

We should return `None` if `None` is set as default value and no input value is provided by the user
I also agree that 
`parser.addini("xy", "", type="linelist", default=[])` :white_check_mark:
while
`parser.addini("xy", "", type="linelist") `:x:

The source code of `parser.addoption` says that this function takes the same arguments as the `add_argument` function of the
argparse library <https://docs.python.org/library/argparse.html>

The default value for the `default` parameter of `add_argument` function is `None`.

So I don't understand why the unit test checks for `[]` as default value.

Commit message: Issue 11282 removed unwanted code to fix coverage

Patch to review:
diff --git a/changelog/11282.bugfix.rst b/changelog/11282.bugfix.rst
new file mode 100644
index 00000000000..7884405f0dc
--- /dev/null
+++ b/changelog/11282.bugfix.rst
@@ -0,0 +1,3 @@
+Fixed returning "None" as the default value for a config option when "None" is explicitly
+provided as the default while creating the config option. If no default value is provided
+at the time of creating the config option, then a default based on the option type is returned
diff --git a/src/_pytest/config/__init__.py b/src/_pytest/config/__init__.py
index 447ebc42abb..24ef491cdb4 100644
--- a/src/_pytest/config/__init__.py
+++ b/src/_pytest/config/__init__.py
@@ -1521,11 +1521,7 @@ def _getini(self, name: str):
             try:
                 value = self.inicfg[name]
             except KeyError:
-                if default is not None:
-                    return default
-                if type is None:
-                    return ""
-                return []
+                return default
         else:
             value = override_value
         # Coerce the values based on types.
diff --git a/src/_pytest/config/argparsing.py b/src/_pytest/config/argparsing.py
index e345de01639..9ac5445e7eb 100644
--- a/src/_pytest/config/argparsing.py
+++ b/src/_pytest/config/argparsing.py
@@ -27,6 +27,14 @@
 FILE_OR_DIR = "file_or_dir"
 
 
+class Notset:
+    def __repr__(self) -> str:
+        return "<notset>"
+
+
+notset = Notset()
+
+
 @final
 class Parser:
     """Parser for command line arguments and ini-file values.
@@ -176,7 +184,7 @@ def addini(
         type: Optional[
             Literal["string", "paths", "pathlist", "args", "linelist", "bool"]
         ] = None,
-        default: Any = None,
+        default: Any = notset,
     ) -> None:
         """Register an ini-file option.
 
@@ -203,6 +211,18 @@ def addini(
         :py:func:`config.getini(name) <pytest.Config.getini>`.
         """
         assert type in (None, "string", "paths", "pathlist", "args", "linelist", "bool")
+        if default is notset:
+            if type is None:
+                default = ""
+            else:
+                if type in ["paths", "pathlist", "args", "linelist"]:
+                    default = []
+                elif type == "bool":
+                    default = False
+                else:
+                    # for string type
+                    default = ""
+
         self._inidict[name] = (help, type, default)
         self._ininames.append(name)
 
diff --git a/testing/test_config.py b/testing/test_config.py
index ded30790188..52fb1e0ae18 100644
--- a/testing/test_config.py
+++ b/testing/test_config.py
@@ -857,6 +857,53 @@ def pytest_addoption(parser):
         assert len(values) == 2
         assert values == ["456", "123"]
 
+    def test_addini_default_values(self, pytester: Pytester) -> None:
+        """Tests the default values for configuration based on
+        config type
+        """
+
+        pytester.makeconftest(
+            """
+            def pytest_addoption(parser):
+                parser.addini("linelist1", "", type="linelist")
+                parser.addini("paths1", "", type="paths")
+                parser.addini("pathlist1", "", type="pathlist")
+                parser.addini("args1", "", type="args")
+                parser.addini("bool1", "", type="bool")
+                parser.addini("string1", "", type="string")
+                parser.addini("none_1", "", type="linelist", default=None)
+                parser.addini("none_2", "", default=None)
+                parser.addini("no_type", "")
+        """
+        )
+
+        config = pytester.parseconfig()
+        # default for linelist, paths, pathlist and args is []
+        value = config.getini("linelist1")
+        assert value == []
+        value = config.getini("paths1")
+        assert value == []
+        value = config.getini("pathlist1")
+        assert value == []
+        value = config.getini("args1")
+        assert value == []
+        # default for bool is False
+        value = config.getini("bool1")
+        assert value is False
+        # default for string is ""
+        value = config.getini("string1")
+        assert value == ""
+        # should return None if None is explicity set as default value
+        # irrespective of the type argument
+        value = config.getini("none_1")
+        assert value is None
+        value = config.getini("none_2")
+        assert value is None
+        # in case no type is provided and no default set
+        # treat it as string and default value will be ""
+        value = config.getini("no_type")
+        assert value == ""
+
     def test_confcutdir_check_isdir(self, pytester: Pytester) -> None:
         """Give an error if --confcutdir is not a valid directory (#2078)"""
         exp_match = r"^--confcutdir must be a directory, given: "


Code review comments:
diff_hunk: @@ -27,6 +27,14 @@
 FILE_OR_DIR = "file_or_dir"
 
 
+class Notset:
+    def __repr__(self) -> str:
+        return "<notset>"
+
+
+notset = Notset()
path: src/_pytest/config/argparsing.py
text: @user1:
Minor, to keep the code standard for globals.

```suggestion
class NotSet:
    def __repr__(self) -> str:
        return "<notset>"


NOT_SET = NotSet()
```

@author:
Thank you Bruno,
I like your suggestions. Will apply the changes and update the PR.

Regards

diff_hunk: @@ -203,6 +211,18 @@ def addini(
         :py:func:`config.getini(name) <pytest.Config.getini>`.
         """
         assert type in (None, "string", "paths", "pathlist", "args", "linelist", "bool")
+        if default is notset:
+            if type is None:
+                default = ""
+            else:
+                if type in ["paths", "pathlist", "args", "linelist"]:
+                    default = []
+                elif type == "bool":
+                    default = False
+                else:
+                    # for string type
+                    default = ""
path: src/_pytest/config/argparsing.py
text: @user1:
Suggestion, move this code to a free function:

```suggestion
        if default is notset:
            default = get_ini_default_for_type(type)
```

And then we can easily unit-test this function, rather than using `pytester`.