Skip to content

Allow int for max_warnings in TOML - #14954

Open
Dmao233 wants to merge 5 commits into
pytest-dev:mainfrom
Dmao233:cursor/max-warnings-toml-int-6b43
Open

Allow int for max_warnings in TOML#14954
Dmao233 wants to merge 5 commits into
pytest-dev:mainfrom
Dmao233:cursor/max-warnings-toml-int-6b43

Conversation

@Dmao233

@Dmao233 Dmao233 commented Aug 31, 2026

Copy link
Copy Markdown

Closes #14953

max_warnings was registered without a type, so native TOML integers (max_warnings = 0) were rejected even though the docs show an unquoted int.

Register it as int | str the same way as truncation_limit_*. _get_max_warnings() treats None as unset so an explicit 0 is not the empty default.

Checklist

  • Include new tests or update existing tests when applicable.
  • Allow maintainers to push and squash when merging my commits.
  • closes #14953 in the PR description and the commit.
  • Changelog file changelog/14953.bugfix.rst.
  • Added myself to AUTHORS in alphabetical order.
  • AI assistance is credited in a Co-authored-by trailer.

The max_warnings option was registered without a type (defaulting to
'string'), so integer values in native TOML config raised a TypeError.
It is now registered with type=int | str, accepting both int and string
values in TOML while keeping the string form working for backward
compatibility. An explicit integer 0 is distinguished from the unset
default.

Co-authored-by: Cursor Grok 4.6 <cursoragent@cursor.com>
@psf-chronographer psf-chronographer Bot added the bot:chronographer:provided (automation) changelog entry is part of PR label Aug 31, 2026

@Pierre-Sassoulas Pierre-Sassoulas left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Looks reasonable thank you.

Comment thread testing/test_warnings.py
result.assert_outcomes(passed=1, warnings=1)
assert result.ret == ExitCode.OK

@pytest.mark.filterwarnings("default::UserWarning")

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

We don't need those automated tests, the int | str mechanism is already tested elsewhere.

Dmao233 and others added 2 commits August 31, 2026 07:21
Co-authored-by: CenFangyu <Dmao233@users.noreply.github.com>
Co-authored-by: CenFangyu <164994318+Dmao233@users.noreply.github.com>
@Amine-LG

Copy link
Copy Markdown

Thanks. I tested the current head, 72679ce2, directly against its base, 5d85267e. Native TOML integer zero is accepted as intended, but I also observed these changes:

Case Base Current head
Option unset getini() returns "" (str) Returns None
INI max_warnings = 0 Returns "0" (str) Returns 0 (int)
-o max_warnings=0 Returns "0" (str) Returns 0 (int)
Empty value in INI/-o, or max_warnings = "" in TOML Treated as unset Reaches int(""), raises ValueError, and exits 1 after the test runs
Native TOML max_warnings = 0 Rejected with exit 4 Enforced with MAX_WARNINGS_ERROR, exit 6

I have three questions:

  1. Are the changes to the existing Config.getini() return values intentional? I reran the str | int, default="" direction described in max_warnings rejects the integer shown in the native TOML documentation #14953, and it accepts native TOML integers while preserving the base values above.

  2. If empty values are now intended to be invalid, should they be rejected as a configuration usage error instead of producing a raw ValueError during session finish after the test has run?

  3. f2141d7 removed the 96-line test block following the review feedback, while 72679ce restored the same block unchanged. Was the restoration intentional? If so, which max_warnings-specific behavior requires the full matrix rather than one focused end-to-end native TOML zero regression plus any compatibility case retained above?

Co-authored-by: CenFangyu <Dmao233@users.noreply.github.com>
@Dmao233

Dmao233 commented Sep 1, 2026

Copy link
Copy Markdown
Author

Addressed in f4d3a6c.

  1. Not intentional. Switched to str | int with default "", so getini keeps the base string/"" behavior; native TOML ints still work.
  2. Empty values stay treated as unset, same as base.
  3. The earlier restore was a mistake. Kept one native TOML zero regression only (test_max_warnings_toml_native_int_zero), dropped the rest per Pierre.

@Amine-LG

Amine-LG commented Sep 1, 2026

Copy link
Copy Markdown

Thanks. I reran f4d3a6ce and confirmed that the base behavior for unset, INI, -o, and empty values is now preserved, while native TOML integer 0 remains accepted and enforced.

This resolves the discrepancies identified in the comparison above.

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.

max_warnings rejects the integer shown in the native TOML documentation

4 participants