feat: add matplotlib requirement factory

[akihikokuroda]

Pull Request

Issue

Fix: #1122

Description

Testing

• Tests added to the respective file if code was changed
• New code has 100% coverage if code was added
• Ensure existing tests and github automation passes (a maintainer will kick off the github automation when the rest of the PR is populated)

Attribution

• AI coding assistants used: claudecode

---

Adding a new component, requirement, sampling strategy, or tool?

If your PR adds or modifies one of the types below, check the matching box. A checklist of type-specific review items will be posted as a comment.

• Component
• Requirement
• Sampling Strategy
• Tool

NOTE: Please ensure you have an issue that has been acknowledged by a core contributor and routed you to open a pull request against this repository. Otherwise, please open an issue before continuing with this pull request.

[akihikokuroda]

@markstur @planetf1 Would you review this? Thanks!

[psschwei]

one question: in #1122 we say that the solution should bundle python_tool_requirements() and accept the allowed_imports parameter, but I don't see those in the PR. just want to confirm that we are explicitly changing the scope (?)

[akihikokuroda]

@psschwei They are missed somehow. I added them and updated tests accordingly. Thanks!

[psschwei]

Review: feat: add matplotlib requirement factory (#1227)

Verdict: Approve with minor suggestions. Clean, well-scoped PR that faithfully mirrors the sibling python_code_generation_requirements() factory. I checked out the branch and verified locally:

• ✅ pytest test/stdlib/requirements/plotting/test_matplotlib.py — 37 passed, 1 xfailed
• ✅ ruff check / ruff format --check — clean
• ✅ mypy on the module — clean
• ✅ Docstring doctests run and pass (8/8)
• ✅ Docstring quality audit — 0 issues (new raises have matching Raises: entries)

The function correctly delegates to python_code_generation_requirements, forwards all params, returns a freshly-allocated list each call (no shared state), and the return order/types match the docstring. No bugs found.

I ran a multi-perspective adversarial review (correctness, API consistency, tests/docs). 7 findings were raised; 4 were refuted on verification. The 3 that survived are all non-blocking:

1. (minor) isinstance/TypeError guard on output_path is inconsistent with the sibling factory

mellea/stdlib/requirements/plotting/matplotlib.py (the if not isinstance(output_path, str): raise TypeError(...) block + the Raises: TypeError entry)

The sibling python_code_generation_requirements() does no isinstance checks and never raises TypeError — it relies on type annotations as the contract, and validates only semantic errors (timeout_seconds <= 0, etc.) with ValueError. PlotFileSaved.__init__(self, output_path: str) likewise doesn't type-check its own arg. The hand-rolled TypeError here guards a contract nothing else in the requirements module guards, and reads as over-engineering per AGENTS.md.

Suggestion: Drop the isinstance/TypeError block, the Raises: TypeError entry, and test_factory_raises_on_non_string_path. Keep the empty/whitespace ValueError guard — that's a genuine semantic check the type system can't catch, mirroring the sibling's positive-int validation. (Optional — if maintainers prefer the defensive guard, it's harmless; just noting the asymmetry.)

2. (nit) Raises: omits ValueErrors from the delegated factory

The Raises: block documents only the TypeError/ValueError raised directly here. But forwarding timeout_seconds/output_limit_chars to python_code_generation_requirements can raise ValueError if either is <= 0, and that propagates through. Not CI-enforced (the docstring gate only checks a function's own raise statements), so this is purely a completeness nicety.

Suggestion: Broaden the ValueError entry, e.g. "If output_path is empty, or if timeout_seconds or output_limit_chars is not positive." Optionally add a small test passing timeout_seconds=0.

3. (nit) test_factory_can_be_unpacked is effectively tautological

assert unpacked == reqs after unpacked = [*reqs] compares identical element references — reflexively true regardless of factory behavior. Only the len(unpacked) == 7 line carries weight, and that's already covered by test_factory_returns_list_of_requirements and test_factory_returns_correct_requirement_order.

Suggestion: Either drop this test or replace the assertion with real 7-tuple destructuring (r1, r2, r3, r4, r5, r6, r7 = reqs) so it actually catches a count regression.

---

None of these block merge — the PR is correct and well-tested. #1 is the one worth a quick decision (align with the sibling factory's no-TypeError convention vs. keep the defensive guard).

[psschwei]

lgtm

[akihikokuroda]

@psschwei Thanks for review. I made these changes.

1. Removed isinstance/TypeError guard (Comment 1)

• Changed: Dropped the isinstance(output_path, str) check and associated TypeError
• Reason: Inconsistent with sibling python_code_generation_requirements(), which relies on type annotations as the contract. Type mismatches fail at call time anyway; the factory should validate only semantic errors like empty strings.
• Test removed: test_factory_raises_on_non_string_path (3 assertions testing TypeError)

2. Expanded Raises docstring (Comment 2)

• Changed: Updated the ValueError entry to document delegated errors:
  ValueError: If output_path is empty or whitespace-only, or if timeout_seconds
  or output_limit_chars is not positive.
• Tests added:
  • test_factory_propagates_timeout_seconds_validation — verifies ValueError from timeout_seconds=0
  • test_factory_propagates_output_limit_chars_validation — verifies ValueError from output_limit_chars=-1

3. Replaced tautological test (Comment 3)

• Changed: test_factory_can_be_unpacked now uses real 7-tuple destructuring instead of reflexive comparison
• Benefit: Unpacking now catches count regressions at runtime; the test fails if the factory returns fewer than 7 requirements

[psschwei]

sorry @akihikokuroda didn't realize that had posted to github 🤦‍♂️ I read it and none of the things it brought up felt like they needed to be raised, so I approved as it was (but I guess github didn't refresh quickly enough for me to see my own review). oh well, YOLO

[akihikokuroda]

I'll merge this. Another PR using this is coming soon.