fix(deck): address PR #759 round-3 review feedback
- Drop "(in-place)" from filter-helper docstrings; callers should consume the return value, mutation is an implementation detail. - Document that deck_get_archived_stacks always returns cards (an archived stack without its cards has no audit value); point to description_max_length for size control. - Document that deck_get_cards applies filtering client-side, so it is network-equivalent to deck_get_stack(include_cards=True). - Pin the empty-list contract: a stack with all-archived cards and include_archived_cards=False yields cards == [] (loaded but empty), not cards is None (explicitly suppressed). - Add explicit one-character-over-limit truncation test alongside the existing exact-boundary test. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
This commit is contained in:
co-authored by
Claude Opus 4.7
parent
b7805c2180
commit
a995155bd4
@@ -57,7 +57,7 @@ def _apply_board_filters(
|
|||||||
include_users: bool,
|
include_users: bool,
|
||||||
include_labels: bool,
|
include_labels: bool,
|
||||||
) -> DeckBoard:
|
) -> DeckBoard:
|
||||||
"""Drop board sub-fields the caller didn't request (in-place)."""
|
"""Drop board sub-fields the caller didn't request; returns the board."""
|
||||||
if not include_acl:
|
if not include_acl:
|
||||||
board.acl = []
|
board.acl = []
|
||||||
if not include_users:
|
if not include_users:
|
||||||
@@ -74,7 +74,7 @@ def _apply_stack_filters(
|
|||||||
include_archived_cards: bool,
|
include_archived_cards: bool,
|
||||||
description_max_length: int | None,
|
description_max_length: int | None,
|
||||||
) -> DeckStack:
|
) -> DeckStack:
|
||||||
"""Apply card-shaping filters to a single stack (in-place)."""
|
"""Apply card-shaping filters to a single stack; returns the stack."""
|
||||||
# Note: the upstream Deck API returns archived cards inline within
|
# Note: the upstream Deck API returns archived cards inline within
|
||||||
# active stacks (the Deck UI filters them frontend-side). Defaulting
|
# active stacks (the Deck UI filters them frontend-side). Defaulting
|
||||||
# include_archived_cards to False mirrors that UI behavior — this is
|
# include_archived_cards to False mirrors that UI behavior — this is
|
||||||
@@ -94,7 +94,7 @@ def _apply_card_filters(
|
|||||||
include_archived_cards: bool,
|
include_archived_cards: bool,
|
||||||
description_max_length: int | None,
|
description_max_length: int | None,
|
||||||
) -> list[DeckCard]:
|
) -> list[DeckCard]:
|
||||||
"""Apply filters to a flat list of cards. Returns a (possibly new) list."""
|
"""Apply filters to a flat list of cards; returns the (possibly new) list."""
|
||||||
if not include_archived_cards:
|
if not include_archived_cards:
|
||||||
cards = [c for c in cards if not c.archived]
|
cards = [c for c in cards if not c.archived]
|
||||||
_truncate_card_descriptions(cards, description_max_length)
|
_truncate_card_descriptions(cards, description_max_length)
|
||||||
@@ -334,6 +334,10 @@ def configure_deck_tools(mcp: FastMCP):
|
|||||||
active board (e.g. cards moved through a "Done" stack and then
|
active board (e.g. cards moved through a "Done" stack and then
|
||||||
archived via deck_archive_card). The shape mirrors deck_get_stacks.
|
archived via deck_archive_card). The shape mirrors deck_get_stacks.
|
||||||
|
|
||||||
|
Cards are always included on the returned stacks (an archived stack
|
||||||
|
without its cards would have no audit value); pass
|
||||||
|
``description_max_length`` if you need to keep the response compact.
|
||||||
|
|
||||||
Args:
|
Args:
|
||||||
board_id: The ID of the board
|
board_id: The ID of the board
|
||||||
description_max_length: If set, truncate each card's description
|
description_max_length: If set, truncate each card's description
|
||||||
@@ -371,6 +375,12 @@ def configure_deck_tools(mcp: FastMCP):
|
|||||||
) -> ListCardsResponse:
|
) -> ListCardsResponse:
|
||||||
"""Get all cards in a Nextcloud Deck stack.
|
"""Get all cards in a Nextcloud Deck stack.
|
||||||
|
|
||||||
|
Filtering is applied client-side after the API returns the full
|
||||||
|
stack, so ``include_archived_cards=False`` and
|
||||||
|
``description_max_length`` reduce response size visible to the
|
||||||
|
caller but not network bandwidth — network-wise this tool is
|
||||||
|
equivalent to deck_get_stack(include_cards=True).
|
||||||
|
|
||||||
Args:
|
Args:
|
||||||
board_id: The ID of the board
|
board_id: The ID of the board
|
||||||
stack_id: The ID of the stack
|
stack_id: The ID of the stack
|
||||||
|
|||||||
@@ -122,6 +122,15 @@ def test_truncate_card_descriptions_at_exact_boundary():
|
|||||||
assert cards[0].description == "x" * 100
|
assert cards[0].description == "x" * 100
|
||||||
|
|
||||||
|
|
||||||
|
def test_truncate_card_descriptions_one_over_limit():
|
||||||
|
"""A description one character over the limit triggers truncation."""
|
||||||
|
cards = [_make_card(1, "x" * 101)]
|
||||||
|
_truncate_card_descriptions(cards, 100)
|
||||||
|
assert cards[0].description is not None
|
||||||
|
assert len(cards[0].description) == 101 # 100 chars + ellipsis
|
||||||
|
assert cards[0].description.endswith("…")
|
||||||
|
|
||||||
|
|
||||||
def test_truncate_card_descriptions_shorter_than_limit_no_ellipsis():
|
def test_truncate_card_descriptions_shorter_than_limit_no_ellipsis():
|
||||||
"""A description shorter than the limit must not have an ellipsis appended."""
|
"""A description shorter than the limit must not have an ellipsis appended."""
|
||||||
cards = [_make_card(1, "hello")]
|
cards = [_make_card(1, "hello")]
|
||||||
@@ -296,6 +305,28 @@ def test_apply_stack_filters_handles_none_cards():
|
|||||||
assert result.cards is None
|
assert result.cards is None
|
||||||
|
|
||||||
|
|
||||||
|
def test_apply_stack_filters_all_archived_yields_empty_list_not_none():
|
||||||
|
"""A stack whose cards are all archived yields cards == [], not None.
|
||||||
|
|
||||||
|
Pin the contract: include_cards=True with all cards filtered out
|
||||||
|
means "the stack was loaded but had nothing to show", which is
|
||||||
|
semantically distinct from include_cards=False (cards=None,
|
||||||
|
"explicitly suppressed"). Callers checking ``stack.cards is None``
|
||||||
|
can use that to distinguish the two states.
|
||||||
|
"""
|
||||||
|
stack = _make_stack(
|
||||||
|
cards=[_make_card(1, archived=True), _make_card(2, archived=True)]
|
||||||
|
)
|
||||||
|
result = _apply_stack_filters(
|
||||||
|
stack,
|
||||||
|
include_cards=True,
|
||||||
|
include_archived_cards=False,
|
||||||
|
description_max_length=None,
|
||||||
|
)
|
||||||
|
assert result.cards == []
|
||||||
|
assert result.cards is not None
|
||||||
|
|
||||||
|
|
||||||
# _apply_card_filters -------------------------------------------------------
|
# _apply_card_filters -------------------------------------------------------
|
||||||
|
|
||||||
|
|
||||||
|
|||||||
Reference in New Issue
Block a user