@sirosen, I ran a review (see https://iscinumpy.dev/post/claude-code-reviews/), below. Would you like me to fix any of these? We should add 3.15 to CI/classifiers since it's in RC phase, for sure.
🤖 AI text below 🤖
Review complete. I read the whole implementation, verified suspicious behaviors by running them, and checked the CLIs. The core resolver (cycle detection, caching, normalization) is sound — I traced the ancestor-tracking logic and could not construct a false-positive or false-negative cycle, including diamonds, self-includes, and resolver reuse after errors. The findings are all at the edges:
Bugs
1. Non-string include-group values produce a cryptic error — _implementation.py:161
The value of {include-group = ...} is never validated as a string. A TOML file with {include-group = 5} fails deep inside _normalize_name with TypeError: expected string or bytes-like object, got 'int', and the lint CLI dutifully reports that unhelpful message. A str check at parse time with a clear message ("include-group value must be a string") would fix both the API and lint output.
2. pip-install-dependency-groups crashes on an empty resolution — _pip_wrapper.py:58
If the requested groups resolve to zero requirements (e.g. empty = []), it runs pip install with no arguments, which fails with a CalledProcessError traceback. Verified. It should skip the pip call (arguably print "nothing to install") when resolved is empty.
3. The dependency-groups CLI shows raw tracebacks for user errors — __main__.py:55
dependency-groups nonexistent prints a full traceback ending in LookupError. Same for invalid TOML or a missing pyproject file (this last one applies to all three CLIs). The lint and pip-install CLIs catch LookupError/ValueError/TypeError and print tidy messages; the main CLI should too.
4. Type hints accept Mapping items but the implementation requires dict — _implementation.py:157
The signatures say Sequence[str | Mapping[str, str]], but _parse_group checks isinstance(item, dict), so a MappingProxyType({"include-group": "b"}) is rejected as "Invalid dependency group item" (verified). Either check Mapping or tighten the hints to dict.
Smaller issues
__main__.main() takes no argv parameter while _lint_dependency_groups.main and _pip_wrapper.main both accept argv for testability. Worth aligning.--list ignores -o/--output and prints group names space-delimited (print(*keys)), which is ambiguous if a name contains a space. Newline-delimited output routed through the same output logic would be more consistent with the rest of the command._toml_compat.py catches ImportError on the first import but ModuleNotFoundError on the second — harmless, but inconsistent for no reason.
Simplifications
_normalize_group_names maintains both original_names (normalized → list of originals) and normalized_to_original (normalized → last original); the second is derivable from the first (names[0] after the duplicate check, since duplicates raise). One structure suffices._parse_group and _resolve both end with "assign to cache dict, then read it back" (self._parsed_groups[group] = tuple(elements); return self._parsed_groups[group]); assigning a local and returning it is simpler.
Modernization
- Python 3.9 is past end-of-life (October 2025).
requires-python = ">=3.9" plus the 3.9 classifier, tox env, and mypy-py39 could be dropped; that would also unlock dataclass(slots=True) and match-free but cleaner typing. This is a maintainer-policy call for a PyPA library, so I'd raise it with sirosen rather than just doing it.
DependencyGroupInclude could be @dataclasses.dataclass(frozen=True) — it's a value type that ends up in cached tuples, so immutability fits (and it becomes hashable).- Python 3.15 is in beta; CI/tox could add it to the test matrix alongside the existing 3.14 coverage.
No performance concerns: the resolver caches parses and resolutions, the 100-deep include chain in the test suite resolves fine, and data sizes here are trivially small.
@sirosen, I ran a review (see https://iscinumpy.dev/post/claude-code-reviews/), below. Would you like me to fix any of these? We should add 3.15 to CI/classifiers since it's in RC phase, for sure.
🤖 AI text below 🤖
Review complete. I read the whole implementation, verified suspicious behaviors by running them, and checked the CLIs. The core resolver (cycle detection, caching, normalization) is sound — I traced the ancestor-tracking logic and could not construct a false-positive or false-negative cycle, including diamonds, self-includes, and resolver reuse after errors. The findings are all at the edges:
Bugs
1. Non-string
include-groupvalues produce a cryptic error —_implementation.py:161The value of
{include-group = ...}is never validated as a string. A TOML file with{include-group = 5}fails deep inside_normalize_namewithTypeError: expected string or bytes-like object, got 'int', and the lint CLI dutifully reports that unhelpful message. A str check at parse time with a clear message ("include-group value must be a string") would fix both the API and lint output.2.
pip-install-dependency-groupscrashes on an empty resolution —_pip_wrapper.py:58If the requested groups resolve to zero requirements (e.g.
empty = []), it runspip installwith no arguments, which fails with aCalledProcessErrortraceback. Verified. It should skip the pip call (arguably print "nothing to install") whenresolvedis empty.3. The
dependency-groupsCLI shows raw tracebacks for user errors —__main__.py:55dependency-groups nonexistentprints a full traceback ending inLookupError. Same for invalid TOML or a missing pyproject file (this last one applies to all three CLIs). The lint and pip-install CLIs catchLookupError/ValueError/TypeErrorand print tidy messages; the main CLI should too.4. Type hints accept
Mappingitems but the implementation requiresdict—_implementation.py:157The signatures say
Sequence[str | Mapping[str, str]], but_parse_groupchecksisinstance(item, dict), so aMappingProxyType({"include-group": "b"})is rejected as "Invalid dependency group item" (verified). Either checkMappingor tighten the hints todict.Smaller issues
__main__.main()takes noargvparameter while_lint_dependency_groups.mainand_pip_wrapper.mainboth acceptargvfor testability. Worth aligning.--listignores-o/--outputand prints group names space-delimited (print(*keys)), which is ambiguous if a name contains a space. Newline-delimited output routed through the same output logic would be more consistent with the rest of the command._toml_compat.pycatchesImportErroron the first import butModuleNotFoundErroron the second — harmless, but inconsistent for no reason.Simplifications
_normalize_group_namesmaintains bothoriginal_names(normalized → list of originals) andnormalized_to_original(normalized → last original); the second is derivable from the first (names[0]after the duplicate check, since duplicates raise). One structure suffices._parse_groupand_resolveboth end with "assign to cache dict, then read it back" (self._parsed_groups[group] = tuple(elements); return self._parsed_groups[group]); assigning a local and returning it is simpler.Modernization
requires-python = ">=3.9"plus the 3.9 classifier, tox env, andmypy-py39could be dropped; that would also unlockdataclass(slots=True)andmatch-free but cleaner typing. This is a maintainer-policy call for a PyPA library, so I'd raise it with sirosen rather than just doing it.DependencyGroupIncludecould be@dataclasses.dataclass(frozen=True)— it's a value type that ends up in cached tuples, so immutability fits (and it becomes hashable).No performance concerns: the resolver caches parses and resolutions, the 100-deep include chain in the test suite resolves fine, and data sizes here are trivially small.