Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

config.getini returns an empty list for an option of type string absent in INI file #11282

Closed
harmin-parra opened this issue Aug 4, 2023 · 8 comments · Fixed by #11594
Closed
Labels
topic: config related to config handling, argument parsing and config file type: bug problem that needs to be addressed

Comments

@harmin-parra
Copy link

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

@The-Compiler
Copy link
Member

Here is an actual reproducer:

conftest.py:

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:

def test_x(my_option):
    assert my_option is None

results in:

    def test_x(my_option):
>       assert my_option is None
E       assert [] is None

@Zac-HD Zac-HD added type: bug problem that needs to be addressed topic: config related to config handling, argument parsing and config file labels Aug 20, 2023
@SharadNair7
Copy link
Contributor

Hello @harmin-parra, @The-Compiler, @Zac-HD,

I can work on this fix if you all agree.

Regards
Sharad

@SharadNair7
Copy link
Contributor

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

@RonnyPfannschmidt
Copy link
Member

the issue lies in

try:
value = self.inicfg[name]
except KeyError:
if default is not None:
return default
if type is None:
return ""
return []

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

@SharadNair7
Copy link
Contributor

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

@SharadNair7
Copy link
Contributor

I personally feel, Solution 1 is the right way to go considering the straight forward and clean approach.

regards
Sharad

@harmin-parra
Copy link
Author

+1 for solution 1

We should return None if None is set as default value and no input value is provided by the user

@harmin-parra
Copy link
Author

harmin-parra commented Oct 2, 2023

I also agree that
parser.addini("xy", "", type="linelist", default=[])
while
parser.addini("xy", "", type="linelist")

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.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
topic: config related to config handling, argument parsing and config file type: bug problem that needs to be addressed
Projects
None yet
Development

Successfully merging a pull request may close this issue.

5 participants