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

Improvement for Parser.addini to facilite parsing of 'int' and 'float' values #13193

Open
wants to merge 20 commits into
base: main
Choose a base branch
from

Conversation

harmin-parra
Copy link

Implements #11381
parsing of int and float configuration values from the ini files has been improved

@psf-chronographer psf-chronographer bot added the bot:chronographer:provided (automation) changelog entry is part of PR label Feb 4, 2025
Copy link
Member

@nicoddemus nicoddemus left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks @harmin-parra for the PR!

Left some tweaks to the docs, otherwise LGTM!

src/_pytest/config/argparsing.py Show resolved Hide resolved
src/_pytest/config/__init__.py Outdated Show resolved Hide resolved
src/_pytest/config/__init__.py Outdated Show resolved Hide resolved
changelog/11381.improvement.rst Outdated Show resolved Hide resolved
changelog/11381.improvement.rst Outdated Show resolved Hide resolved
changelog/11381.improvement.rst Outdated Show resolved Hide resolved
changelog/11381.improvement.rst Outdated Show resolved Hide resolved
@harmin-parra
Copy link
Author

There are still some mypy errors that I have no idea how to resolve

Copy link
Member

@nicoddemus nicoddemus left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Awesome, thanks!

I will leave it open for a few days to give others a chance to review.

Copy link
Member

@The-Compiler The-Compiler left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think all those casts in config/__init__.py are incorrect and are hiding bugs.

A pyproject.toml with:

[tool.pytest.ini_options]
int_value = [3]

now causes

        elif type == "int":
            # value = cast(str, value)
            try:
>               return int(value)
                       ^^^^^^^^^^
E               TypeError: int() argument must be a string, a bytes-like object or a real number, not 'list'

I suspect (but haven't tried) that passing an int/float for a paths option will fail in a similar way.

We should probably do some additional validation that the input-type is correct, and raise an exception including the option name if not.

@nicoddemus
Copy link
Member

@The-Compiler I agree.

Currently it is only catching ValueError, perhaps it might be enough to catch TypeError too.

@The-Compiler
Copy link
Member

The-Compiler commented Feb 5, 2025

That would still need a cast that's actually not true to appease mypy. IMHO, an explicit:

if not isinstance(value, (str, int)):
    raise TypeError(f"Expected str or int for option {name} of type integer, but got: {value!r} ({type(value)})")

or so would:

  • Make mypy happy (I think?) without needing a cast that's not actually true
  • Give a nicer error message and information to the user
  • Make sure we don't do any other operations withe the value that might raise e.g. AttributeError rather than TypeError (but we would not statically catch because we lied to mypy)
  • Avoid confusion for others reading the code later because we cast a value to a str when it actually isn't

@nicoddemus
Copy link
Member

Sounds good.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bot:chronographer:provided (automation) changelog entry is part of PR
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants