Allow int for truncation_limit_lines and truncation_limit_chars in TOML - #14692
Allow int for truncation_limit_lines and truncation_limit_chars in TOML#14692Pierre-Sassoulas wants to merge 2 commits into
truncation_limit_lines and truncation_limit_chars in TOML#14692Conversation
bluetech
left a comment
There was a problem hiding this comment.
We can't just change to int, as that would be a breaking change. We need to allow both, unfortunately.
I will write in the issue how I think we can do it.
4010bfd to
007af70
Compare
nicoddemus
left a comment
There was a problem hiding this comment.
Overall nice work.
It is different than the original proposal of using the types directly (str | int vs ("str", "int")), but I suspect this is good enough and aligns well with the original API.
| #: a value of any of these types" (e.g. ``("int", "string")``). | ||
| IniType = _IniTypeTag | tuple[_IniTypeTag, ...] | ||
|
|
||
| _INI_TYPE_TAGS: tuple[str, ...] = ( |
There was a problem hiding this comment.
| _INI_TYPE_TAGS: tuple[str, ...] = ( | |
| #: Runtime list tags accepted by :meth:`Parser.addini` for its ``type`` argument. | |
| #: Keep in sync with _IniTypeTag. | |
| _INI_TYPE_TAGS: tuple[str, ...] = ( |
|
|
||
| FILE_OR_DIR = "file_or_dir" | ||
|
|
||
| #: The string tags accepted by :meth:`Parser.addini` for its ``type`` argument. |
There was a problem hiding this comment.
| #: The string tags accepted by :meth:`Parser.addini` for its ``type`` argument. | |
| #: The string tags accepted by :meth:`Parser.addini` for its ``type`` argument. | |
| #: Keep in sync with _INI_TYPE_TAGS. |
| if type is None: | ||
| type = "string" | ||
| tags = type if isinstance(type, tuple) else (type,) | ||
| assert tags and all(tag in _INI_TYPE_TAGS for tag in tags), type |
There was a problem hiding this comment.
We probably should use an exception here, given this might be an user-provided value.
asserts should be used for internal checks/consistency only.
| if type in ("paths", "pathlist", "args", "linelist"): | ||
| if isinstance(type, tuple): | ||
| # A union has no unambiguous implicit default; require an explicit one. | ||
| return None |
There was a problem hiding this comment.
IIUC, None will be used as default, contrary to the comment. Perhaps raise an exception instead?
Address review feedback in pytest-dev#14692: - Replace the tuple-of-tags API with type expressions, as proposed by bluetech in pytest-dev#14675: plain Python types (str, bool, int, float) and unions of them (e.g. `int | str`) are now accepted in addition to the existing string tags. The truncation options are registered with `type=int | str`. - Raise ValueError instead of asserting on an invalid `type` argument, since it is user (plugin) provided. - Raise ValueError when a union type is registered without an explicit default, instead of silently defaulting to None. - Document the _IniTypeTag/_INI_TYPE_TAGS keep-in-sync requirement.
007af70 to
353934c
Compare
|
Thank you for the review ! I think I addressed all comments in 353934c, also I added support for direct |
|
Maybe it would be better to separate in two PRs starting by the refactor to allow multiple type though, let me know. |
353934c to
6942da0
Compare
|
Depends on #14751 |
6942da0 to
501698d
Compare
501698d to
5e48fc9
Compare
|
This is reviewable again since we merged #14751 |
bluetech
left a comment
There was a problem hiding this comment.
Thanks.
We also need to update the API Reference (which it seems is currently falsely stating type int...)
It would also be nice to audit the other configuration options to see if they need similar treatment, but we can do that separately.
| max_chars: int = DEFAULT_MAX_CHARS | ||
|
|
||
| @classmethod | ||
| def from_config(cls, config: Config) -> TruncationBudget: |
There was a problem hiding this comment.
Since this introduces a cyclic dependency, maybe the previous location is better.
| def from_config(cls, config: Config) -> TruncationBudget: | ||
| """Build a budget from the ``truncation_limit_*`` ini options. | ||
|
|
||
| Both options are registered with ``type=int | str`` for |
There was a problem hiding this comment.
I think this comment belongs on the addini, not here. Can also be shorter 😉
The truncation_limit_lines and truncation_limit_chars options were registered without a type (defaulting to 'string'), so integer values in native TOML config raised a TypeError (pytest-dev#14675). They are now registered with type=int | str, accepting both int and string values in TOML while keeping the string form working for backward compatibility. TruncationBudget.from_config reads the two options and owns default handling: None (option unset) falls back to the DEFAULT_MAX_* limits, which live next to the class; other values are coerced with int(). truncate_if_required short-circuits on verbosity/CI before reading the options at all. 0 still means 'unbounded'; None means 'use default'. Fixes pytest-dev#14675 Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
The API reference claimed type int while the options were registered as string; the registration is now int | str, so state that in the confval entries and refresh the stale --help transcript accordingly. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
5e48fc9 to
6e02b69
Compare
The truncation_limit_lines and truncation_limit_chars ini options were registered without a type, defaulting to 'string'. In native TOML config ([tool.pytest]), integer values then raised a TypeError instead of being accepted, despite being numeric limits.
Register both options with type='int' so TOML integer values are accepted (INI string values keep working via the existing int coercion).
Basically #14689 but without unwanted AI to human conversation.
Fixes #14675