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

[harmin-parra]

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

[nicoddemus]

Thanks @harmin-parra for the PR!

Left some tweaks to the docs, otherwise LGTM!

[harmin-parra]

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

[nicoddemus]

Awesome, thanks!

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

[The-Compiler]

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]

@The-Compiler I agree.

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

[The-Compiler]

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]

Sounds good.

[nicoddemus]

@The-Compiler applied the explicit checks. 👍

[The-Compiler]

Two small things about the type checks, other than that this seems good to go.

I think it would be good to have some tests for those error conditions because it took me a while to understand how those types could even get into the function (inicfg didn't sound like it could be a typed TOML file). But if you feel like this is getting into too much of a tangent for this PR, I can understand.

[nicoddemus]

Looking deeper I noticed that the previous annotations adding int|float to the signatures were incorrect: when we parse TOML files we always convert to str/list[str] for compatibility:

pytest/src/_pytest/config/findpaths.py

Lines 85 to 91 in b28195f

	# TOML supports richer data types than ini files (strings, arrays, floats, ints, etc),
	# however we need to convert all scalar values to str for compatibility with the rest
	# of the configuration system, which expects strings only.
	def make_scalar(v: object) -> str | list[str]:
	return v if isinstance(v, list) else str(v)
	return {k: make_scalar(v) for k, v in result.items()}

[The-Compiler]

Oh wow, good catch! I only tested with [0] which of course was preserved as a list, so I assumed that's what happens at runtime with the other types too!

If they are indeed all strings, I believe those isinstance() checks can all be dropped again then, modulo ensuring that we have str and not list for a float / int?

Though I wonder what happens with:

        elif type == "bool":
            return _strtobool(str(value).strip())
        elif type == "string":
            return value

if someone passes in a list value there?

[nicoddemus]

If they are indeed all strings, I believe those isinstance() checks can all be dropped again then, modulo ensuring that we have str and not list for a float / int?

Ouch, of course, thanks for catching that. I fixed and improved he checks to at least provide a better message than the default (int() argument must be a string, a bytes-like object or a real number, not 'list').

Though I wonder what happens with

_strtobool validates the object for us:

pytest/src/_pytest/config/__init__.py

Lines 1885 to 1900 in b302d31

	def _strtobool(val: str) -> bool:
	"""Convert a string representation of truth to True or False.
	True values are 'y', 'yes', 't', 'true', 'on', and '1'; false values
	are 'n', 'no', 'f', 'false', 'off', and '0'. Raises ValueError if
	'val' is anything else.
	.. note:: Copied from distutils.util.
	"""
	val = val.lower()
	if val in ("y", "yes", "t", "true", "on", "1"):
	return True
	elif val in ("n", "no", "f", "false", "off", "0"):
	return False
	else:
	raise ValueError(f"invalid truth value {val!r}")

But indeed elif type == "string" would erroneously return a list -- but I'm not certain if we should really touch that right now.

[nicoddemus]

@The-Compiler gentle ping. 👍