eops-378(fix): Fix using --all display a warning letting the user know there are more records if we hit the limit - #23
Conversation
…w there are more records if we hit the limit
| Fetch pages and return {items, count}. | ||
|
|
||
| ``count`` is the API total. When the page ceiling (or ``max_items``) stops | ||
| the crawl early, ``next_offset`` is set so callers can resume. | ||
| """ | ||
| params = dict(params or {}) | ||
| params["limit"] = page_size | ||
| params["offset"] = 0 | ||
| all_items = [] | ||
| max_pages = 1000 | ||
| params["offset"] = start_offset | ||
| all_items: list = [] | ||
| total = 0 |
There was a problem hiding this comment.
[Medium] paginate() docstring does not clarify that count is always the API total, not fetched count
When truncation occurs (line 241 or 246), the result includes next_offset to signal resumption. The caller at commands/__init__.py:119 passes data.get("count", next_offset) as the denominator in the warning message. If a future caller treats count as the number of items fetched (not the API total), the resume hint will misreport progress. The current code is correct because count always holds the API total, but this contract is implicit.
Command: clarify the docstring at client.py:223 to state that count is always the API total, regardless of truncation.
| Fetch pages and return {items, count}. | |
| ``count`` is the API total. When the page ceiling (or ``max_items``) stops | |
| the crawl early, ``next_offset`` is set so callers can resume. | |
| """ | |
| params = dict(params or {}) | |
| params["limit"] = page_size | |
| params["offset"] = 0 | |
| all_items = [] | |
| max_pages = 1000 | |
| params["offset"] = start_offset | |
| all_items: list = [] | |
| total = 0 | |
| def paginate( | |
| self, | |
| path: str, | |
| params: dict[str, Any] | None = None, | |
| page_size: int = 100, | |
| max_items: int | None = None, | |
| *, | |
| start_offset: int = 0, | |
| ) -> dict: | |
| """ | |
| Fetch pages and return {items, count, next_offset?}. | |
| ``count`` is always the API total, regardless of truncation. | |
| When the page ceiling (or ``max_items``) stops the crawl early, | |
| ``next_offset`` is set so callers can resume. | |
| """ |
There was a problem hiding this comment.
Stale — paginate docstring already states count is always the API total, regardless of truncation (317c799).
| for _ in range(_MAX_PAGES): | ||
| data = self.get(path, params=params) | ||
| items = data.get("items", []) | ||
| all_items.extend(items) | ||
| total = data.get("count", len(items)) | ||
| total = data.get("count", start_offset + len(all_items)) | ||
| if max_items and len(all_items) >= max_items: | ||
| all_items = all_items[:max_items] | ||
| truncated = start_offset + len(all_items) < total | ||
| break | ||
| if len(all_items) >= total or not items: | ||
| if start_offset + len(all_items) >= total or not items: | ||
| break | ||
| params["offset"] += page_size | ||
| else: | ||
| truncated = start_offset + len(all_items) < total |
There was a problem hiding this comment.
[Medium] truncated flag is set twice: redundant assignment in the for-else block when max_items breaks early
Line 241 sets truncated = start_offset + len(all_items) < total inside the if max_items block. If this condition triggers, the loop breaks (line 242) and the for-else at line 246 never runs — but the else re-assigns the same truncated value. The logic is correct by accident (both branches compute the same value), but the duplicate obscures the intent. Hoist the initialization and rely on one assignment.
Command: restructure the truncation logic at client.py:235 to eliminate the redundant assignment.
| for _ in range(_MAX_PAGES): | |
| data = self.get(path, params=params) | |
| items = data.get("items", []) | |
| all_items.extend(items) | |
| total = data.get("count", len(items)) | |
| total = data.get("count", start_offset + len(all_items)) | |
| if max_items and len(all_items) >= max_items: | |
| all_items = all_items[:max_items] | |
| truncated = start_offset + len(all_items) < total | |
| break | |
| if len(all_items) >= total or not items: | |
| if start_offset + len(all_items) >= total or not items: | |
| break | |
| params["offset"] += page_size | |
| else: | |
| truncated = start_offset + len(all_items) < total | |
| all_items: list = [] | |
| total = 0 | |
| truncated = False | |
| for _ in range(_MAX_PAGES): | |
| data = self.get(path, params=params) | |
| items = data.get("items", []) | |
| all_items.extend(items) | |
| total = data.get("count", start_offset + len(all_items)) | |
| if max_items and len(all_items) >= max_items: | |
| all_items = all_items[:max_items] | |
| truncated = start_offset + len(all_items) < total | |
| break | |
| if start_offset + len(all_items) >= total or not items: | |
| break | |
| params["offset"] += page_size | |
| else: | |
| truncated = start_offset + len(all_items) < total |
There was a problem hiding this comment.
Stale — one truncate check after the loop already replaced the double assignment (317c799).
| # Map _do_list filter kwargs to CLI flags for the --all resume hint. | ||
| _FILTER_CLI_FLAGS = ( | ||
| ("search", "--search"), | ||
| ("status", "--status"), | ||
| ("start_date", "--start-date"), | ||
| ("end_date", "--end-date"), | ||
| ("company_id", "--company"), | ||
| ("customer_id", "--customer"), | ||
| ("vendor_id", "--vendor"), | ||
| ) | ||
|
|
There was a problem hiding this comment.
_FILTER_CLI_FLAGS stores filter-to-flag mappings as a positional tuple, which is error-prone and O(N) on every resume command. Siblings use a dict for this pattern.
Command: convert _FILTER_CLI_FLAGS to a dict at commands/__init__.py:75.
| # Map _do_list filter kwargs to CLI flags for the --all resume hint. | |
| _FILTER_CLI_FLAGS = ( | |
| ("search", "--search"), | |
| ("status", "--status"), | |
| ("start_date", "--start-date"), | |
| ("end_date", "--end-date"), | |
| ("company_id", "--company"), | |
| ("customer_id", "--customer"), | |
| ("vendor_id", "--vendor"), | |
| ) | |
| _FILTER_CLI_FLAGS = { | |
| "search": "--search", | |
| "status": "--status", | |
| "start_date": "--start-date", | |
| "end_date": "--end-date", | |
| "company_id": "--company", | |
| "customer_id": "--customer", | |
| "vendor_id": "--vendor", | |
| } |
There was a problem hiding this comment.
Stale — _FILTER_CLI_FLAGS is already a dict (317c799).
|
|
||
|
|
||
| def _warn_all_truncated(path: str, *, fetched_through: int, total: int, next_offset: int, filters: dict) -> None: | ||
| """Tell the user --all stopped early and how to continue.""" |
There was a problem hiding this comment.
_resume_all_command checks truthiness of value instead of is not None, which silently drops falsy filters like status=0 or company_id=False.
Command: tighten the filter check at commands/__init__.py:98 to check is not None.
| """Tell the user --all stopped early and how to continue.""" | |
| if value is not None: |
There was a problem hiding this comment.
Stale — resume hint already uses value is not None (317c799).
| fg=typer.colors.YELLOW, | ||
| err=True, | ||
| ) | ||
|
|
There was a problem hiding this comment.
_warn_all_truncated imports _MAX_PAGES inside the function body instead of at module load. This defers the import until the warning fires and breaks if the client module is broken.
Command: move the import to the top of commands/__init__.py at the module-level imports.
| from dualentry_cli.client import DualEntryClient, _MAX_PAGES |
There was a problem hiding this comment.
Stale — _MAX_PAGES is imported at module top of commands/__init__.py (317c799).
Keep API total as count. One truncate check after the page cap. Refs EOPS-378.
Resume --all reports the cursor, not this-run size.
Warkanlock
left a comment
There was a problem hiding this comment.
CI green. Review comments addressed or replied.
Keep both make_list_command_cls (EOPS-369) and _MAX_PAGES (EOPS-378) after #23 landed.
Summary
Display a warning when using the
--allflag if there are more recordsThe warning provides information on how to fetch more records
Changes
Test plan
uv run pytest)uv run ruff check .)dualentry <command>