Skip to content

Allow int for max_warnings in TOML - #14954

Merged
bluetech merged 1 commit into
pytest-dev:mainfrom
Dmao233:cursor/max-warnings-toml-int-6b43
Sep 6, 2026
Merged

Allow int for max_warnings in TOML#14954
bluetech merged 1 commit into
pytest-dev:mainfrom
Dmao233:cursor/max-warnings-toml-int-6b43

Conversation

@Dmao233

@Dmao233 Dmao233 commented Aug 31, 2026 •
edited
Loading

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.

@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 requested changes Aug 31, 2026
View reviewed changes

@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 Outdated
@Amine-LG

Amine-LG commented Aug 31, 2026

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?

@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.

@Dmao233

Dmao233 commented Sep 3, 2026

Copy link
Copy Markdown
Author

Addressed in f4d3a6c. Amine confirmed base behavior is preserved and native TOML int 0 still works. Ready for another look.

@bluetech
bluetech force-pushed the cursor/max-warnings-toml-int-6b43 branch 2 times, most recently from 0b7c71f to de7bf7f Compare September 5, 2026 18:51
bluetech
bluetech approved these changes Sep 5, 2026
View reviewed changes

@bluetech bluetech 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.

Thanks. I made some tweaks.

(@Pierre-Sassoulas I kept a test otherwise we won't have coverage for the branch.)

Pierre-Sassoulas
Pierre-Sassoulas approved these changes Sep 5, 2026
View reviewed changes

@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.

LGTM !

@cursoragent @bluetech
Allow int for max_warnings in TOML (pytest-dev#14953)
ee08c00 
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 <[email protected]>
@bluetech
bluetech force-pushed the cursor/max-warnings-toml-int-6b43 branch from de7bf7f to ee08c00 Compare September 5, 2026 19:07
@bluetech
bluetech merged commit b0ca559 into pytest-dev:main Sep 6, 2026
36 checks passed
@RonnyPfannschmidt RonnyPfannschmidt mentioned this pull request Oct 9, 2026
Merged
4 tasks done
SemTiOne pushed a commit to SemTiOne/pytest that referenced this pull request Oct 9, 2026
Ronny Pfannschmidt @claude
Accept native TOML types for the remaining built-in ini options
69890d2 
Follow-up to pytest-dev#14692, pytest-dev#14954 and pytest-dev#15133: the remaining options whose
values are naturally numbers or booleans were still registered as plain
strings, so native TOML configs had to quote them.

- verbosity_assertions, verbosity_test_cases, verbosity_subtests:
  int | Literal["auto"]
- tmp_path_retention_count: int | str
- log_level, log_cli_level, log_file_level: int | str; int levels are
  now handled directly and an int 0 (NOTSET) still counts as set
- log_auto_indent: int | bool | str
- log_file_mode: Literal["w", "a"] (an invalid value was an INTERNALERROR)
- parametrize_long_str_id_strategy: its Literal choices

The reference docs drop the "must be quoted in TOML" notes, fix the
minversion TOML example (a float there is rejected), fix copy-pasted
log_cli_level examples, and refresh the --help dump.

Co-Authored-By: Claude Opus 5.5 (1M context) <[email protected]>
Claude-Session: https://claude.ai/code/session_01KnFJBdcVQB59PKYw3q1w7X
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@bluetech bluetech bluetech approved these changes
@Pierre-Sassoulas Pierre-Sassoulas Pierre-Sassoulas approved these changes

Assignees

No one assigned

Labels

bot:chronographer:provided (automation) changelog entry is part of PR

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

max_warnings rejects the integer shown in the native TOML documentation

5 participants

@Dmao233 @Amine-LG @bluetech @Pierre-Sassoulas @cursoragent