Skip to content

test: add coverage for tomlkit._types wrapper operators - #570

Open
darrenhuai wants to merge 1 commit into
python-poetry:masterfrom
darrenhuai:test/cover-types-wrapper-operators
Open

test: add coverage for tomlkit._types wrapper operators#570
darrenhuai wants to merge 1 commit into
python-poetry:masterfrom
darrenhuai:test/cover-types-wrapper-operators

Conversation

@darrenhuai

Copy link
Copy Markdown

Summary

tomlkit/_types.py was sitting at 60% coverage. The uncovered part is _CustomList.__add__/__iadd__, _CustomDict.__or__/__ior__, and the wrap_method helper — the machinery every Array and table type relies on to get list/dict operator support. None of it had a direct test.

While writing these I noticed something worth calling out: Array + [...] returns a plain list, not an Array (the .copy() in __add__ doesn't preserve the subclass), whereas Table | {...} does come back as a Table. That's existing behavior, not something I changed — I just added a test that pins it down instead of leaving it implicit. Worth a look if that asymmetry wasn't intentional, but out of scope for a coverage PR.

wrap_method isn't called anywhere in the current codebase but is exported in __all__, so I tested it directly against a small stand-in class.

_types.py coverage: 60% → 100%. Full suite (1057 tests) still green, ruff and mypy --strict both clean.

Agent Drafting Metadata

  • Agent: Claude Code
  • Model: claude-sonnet-5
  • Notes: I used Claude Code to find the coverage gap and draft these tests. Ran the full test suite, ruff, and mypy myself before pushing, and read through the diff.

_CustomList.__add__/__iadd__, _CustomDict.__or__/__ior__, and the
wrap_method helper had no direct tests, leaving tomlkit/_types.py at
60% coverage despite backing the operator overloads on Array and
every table type. Notably, __add__ silently returns a plain list
(loses the Array wrapper) while __or__ preserves the Table wrapper -
this asymmetry is now pinned down by a test instead of being
implicit. wrap_method itself is unused elsewhere in the codebase but
is part of the public API (exported via __all__), so it's tested
directly against a minimal wrapper class.

tomlkit/_types.py coverage: 60% -> 100%.

Agent: Claude Code
Model: claude-sonnet-5
Notes: repo research, coverage-gap analysis, tests, and verification
(pytest/ruff/mypy) all done by the agent; reviewed before pushing.
@dimbleby

Copy link
Copy Markdown
Contributor

wrap_method should have been removed at #460, better to remove it now than test it

@davidpavlovschi davidpavlovschi left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

The operator coverage is useful, and the __iadd__, __or__, and __ior__ cases match the implementations.

Two changes before this should merge:

  1. Please remove wrap_method and its __all__ entry instead of adding tests for it, as noted above. It has no callers and appears to be leftover from #460.
  2. This is framed as a coverage-only PR, but assert not isinstance(result, Array) permanently specifies the wrapper-dropping behavior that the description itself flags as potentially unintended. Please either establish that a plain list is the intended API before pinning it, or keep the regression test focused on the combined values without asserting the result type.

That keeps the PR from turning two unresolved implementation details into supported behavior solely to reach 100% line coverage.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants