Support tuple items in map#17
Conversation
There was a problem hiding this comment.
PR Review
PR: Support tuple items in map
Important
Verdict: Request changes - 6 actionable findings, highest severity P1.
Findings (6)
Breaking change: tuple items now unpacked automatically
src/cashet/_batch.py:85
The new logic in build_map_tasks unpacks any tuple item with *item, treating each element as a separate positional argument. This silently breaks callers who previously used map with tuple items that were meant to be passed as a single argument. The old behavior treated item as a single argument regardless of type. Consider documenting this as a breaking change or adding a parameter to control unpacking.
Namedtuple items incorrectly unpacked
src/cashet/_batch.py:85
Namedtuples are instances of tuple, so isinstance(item, tuple) is True. Passing a list of namedtuples will unpack each field as separate arguments, which is probably not the intended behavior. Users might expect namedtuples to be treated as single items. Either document this as intentional or change the check to isinstance(item, tuple) and not namedtuple.
Empty tuple items lead to missing arguments
src/cashet/_batch.py:85
If an empty tuple () is passed as an item, *item expands to nothing, so the resulting task will have only the extra *args and kwargs, likely causing TypeError for functions that require at least one positional argument. The previous behavior passed the empty tuple as a single argument. This edge case may cause unexpected errors.
build_map_tasks lacks docstring
src/cashet/_batch.py:78
The function should include a docstring explaining its behavior, especially the tuple unpacking logic, to help maintainability.
Imprecise return type in build_map_tasks
src/cashet/_batch.py:83
The return type is list[Any], which loses information. Consider using list[tuple[Callable[..., Any], tuple[Any, ...], dict[str, Any]]] for better type safety.
Missing edge case tests for tuple unpacking
tests/test_map.py:46
The new test only covers a tuple of two elements. Consider adding tests for empty tuple, single-element tuple, and mixed non-tuple/tuple items to ensure correctness.
How To Recheck
Reply @ds-review recheck under the relevant inline finding after pushing a fix.
| kwargs: dict[str, Any], | ||
| ) -> list[Any]: | ||
| return [ | ||
| (func, (*item, *args) if isinstance(item, tuple) else (item, *args), kwargs) |
There was a problem hiding this comment.
Breaking change: tuple items now unpacked automatically
The new logic in build_map_tasks unpacks any tuple item with *item, treating each element as a separate positional argument. This silently breaks callers who previously used map with tuple items that were meant to be passed as a single argument. The old behavior treated item as a single argument regardless of type. Consider documenting this as a breaking change or adding a parameter to control unpacking.
| kwargs: dict[str, Any], | ||
| ) -> list[Any]: | ||
| return [ | ||
| (func, (*item, *args) if isinstance(item, tuple) else (item, *args), kwargs) |
There was a problem hiding this comment.
Namedtuple items incorrectly unpacked
Namedtuples are instances of tuple, so isinstance(item, tuple) is True. Passing a list of namedtuples will unpack each field as separate arguments, which is probably not the intended behavior. Users might expect namedtuples to be treated as single items. Either document this as intentional or change the check to isinstance(item, tuple) and not namedtuple.
| kwargs: dict[str, Any], | ||
| ) -> list[Any]: | ||
| return [ | ||
| (func, (*item, *args) if isinstance(item, tuple) else (item, *args), kwargs) |
There was a problem hiding this comment.
Empty tuple items lead to missing arguments
If an empty tuple () is passed as an item, *item expands to nothing, so the resulting task will have only the extra *args and kwargs, likely causing TypeError for functions that require at least one positional argument. The previous behavior passed the empty tuple as a single argument. This edge case may cause unexpected errors.
| return normalized | ||
|
|
||
|
|
||
| def build_map_tasks( |
| items: Iterable[Any], | ||
| args: tuple[Any, ...], | ||
| kwargs: dict[str, Any], | ||
| ) -> list[Any]: |
There was a problem hiding this comment.
Imprecise return type in build_map_tasks
The return type is list[Any], which loses information. Consider using list[tuple[Callable[..., Any], tuple[Any, ...], dict[str, Any]]] for better type safety.
| ) -> list[Any]: | |
| ) -> list[tuple[Callable[..., Any], tuple[Any, ...], dict[str, Any]]]: |
| assert [r.load() for r in refs] == [11, 12, 13] | ||
|
|
||
| def test_map_with_tuple_items(self, client: Client) -> None: | ||
| refs = client.map(add_pair, [(1, 2), (3, 4)]) |
Summary
map()task construction into the shared batch helpermap()to be used as positional argumentsTesting
uv run ruff check src/ tests/uv run pyright src/uv run pytest tests/ -v