fix: apply --limit to the write, not to the selection
pull.py's docstring said "the limit is on the write, not on the selection", and the code did the opposite: `list_issues` truncated the payload list to `limit`, and pull.py dropped the closed ones after that. A milestone whose first issues are closed therefore answered `--limit 20` with twelve files, and the only statement about the behavior anywhere was the false one. The limit now counts what the run leaves in the store. `list_issues` takes a `keep` predicate, pages keep arriving until `limit` payloads have satisfied it, and the ones that did not are still returned — they were enumerated, and pull.py still reports them as "N closed, not stored". What `keep` means stays the caller's business; the transport only counts. pull.py hands it `lands_in_store`, which is the same test the walk itself applies: a closed issue counts only when the store already has it, since that one is refreshed rather than dropped. Pagination is the other half, and it cuts both ways. `paginate` is now a thin wrapper over a new `pages` generator, so the page after the one that fills the budget is never requested. In the other direction "fetch until N are kept" is "fetch the whole tracker" on a filter that matches mostly closed issues, so a keep-bounded read scans at most PAGE_SLACK times the pages the limit would need if nothing were dropped, then warns on stderr and returns short. Raising --limit raises that ceiling with it. --deps is outside the count: a dependency is followed because an issue named it. remote.py keeps the old meaning and now says so in as many words — it writes nothing, so there is no write for a limit to bound, and its --limit caps the listing, closed issues included. Same flag, two jobs, documented in both scripts and in the skill's command table. Also refuses `--limit 0` instead of dividing by the page size and raising ZeroDivisionError. tests/test_pull_limit.py stubs the transport with a fake that serves `page=`/`limit=` itself, so the request pattern is observed rather than assumed: exactly N files out of a half-closed selection, the second page fetched and the third not, the scan stopping at the budget with a warning, and remote.py's listing unchanged. 251 tests, no network. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
This commit is contained in:
@@ -30,6 +30,13 @@ import urllib.parse
|
||||
PAYLOAD_DIR = ".payload"
|
||||
REMOTE_MAP = ".remote.json"
|
||||
|
||||
# How far past the ideal page count a `keep`-bounded listing may scan before it
|
||||
# gives up (see list_issues). The ideal is what `limit` would need if every
|
||||
# payload counted; the slack pays for the ones that do not. It is a bound on
|
||||
# requests, deliberately small: "fetch until N are kept" without one is "fetch
|
||||
# the whole tracker" on any repo whose filter matches mostly closed issues.
|
||||
PAGE_SLACK = 4
|
||||
|
||||
|
||||
def die(msg, code=1):
|
||||
sys.stderr.write("%s: %s\n" % (os.path.basename(sys.argv[0]), msg))
|
||||
@@ -114,17 +121,28 @@ def api(login, endpoint, method="GET", payload=None, payload_name=None,
|
||||
die("`tea api %s` returned non-JSON:\n%s" % (endpoint, body[:500]))
|
||||
|
||||
|
||||
def paginate(login, endpoint, limit=50, max_pages=40, **kw):
|
||||
"""GET a list endpoint page by page; return the concatenated list."""
|
||||
def pages(login, endpoint, limit=50, max_pages=40, **kw):
|
||||
"""GET a list endpoint page by page, yielding each page as it arrives.
|
||||
|
||||
A generator, because a caller whose budget is spent on what it *keeps*
|
||||
cannot be served by a function that fetches everything first: the page after
|
||||
the one that completed the budget must never be requested. Stop consuming
|
||||
and no further request is made."""
|
||||
sep = "&" if "?" in endpoint else "?"
|
||||
out = []
|
||||
for page in range(1, max_pages + 1):
|
||||
batch = api(login, "%s%spage=%d&limit=%d" % (endpoint, sep, page, limit), **kw)
|
||||
if not isinstance(batch, list) or not batch:
|
||||
break
|
||||
out.extend(batch)
|
||||
return
|
||||
yield batch
|
||||
if len(batch) < limit:
|
||||
break
|
||||
return # a short page is the last one
|
||||
|
||||
|
||||
def paginate(login, endpoint, limit=50, max_pages=40, **kw):
|
||||
"""GET a list endpoint page by page; return the concatenated list."""
|
||||
out = []
|
||||
for batch in pages(login, endpoint, limit=limit, max_pages=max_pages, **kw):
|
||||
out.extend(batch)
|
||||
return out
|
||||
|
||||
|
||||
@@ -187,11 +205,31 @@ def matches(payload, milestone_id=None, labels=()):
|
||||
|
||||
|
||||
def list_issues(login, base, state="open", labels=(), query=None,
|
||||
milestone=None, limit=100):
|
||||
milestone=None, limit=100, keep=None):
|
||||
"""Filtered issue payloads. Returns (payloads, milestone_title).
|
||||
|
||||
One request per page, and the payload already carries the issue bodies — a
|
||||
whole milestone costs one call per 50 issues, not one per issue."""
|
||||
whole milestone costs one call per 50 issues, not one per issue.
|
||||
|
||||
`limit` counts the payloads the CALLER cares about, not the ones the server
|
||||
returned. Without `keep` those are the same thing and this behaves as it
|
||||
always did. With it, `keep(payload)` says whether a payload counts, pages
|
||||
keep coming until `limit` of them have, and the returned list carries the
|
||||
ones that did not count too — they were enumerated, and a caller that has
|
||||
something to say about them (pull.py: "N closed, not stored") still can.
|
||||
|
||||
What `keep` means is the caller's business; this module only counts. Two
|
||||
boundaries hold whatever it decides:
|
||||
|
||||
- **Stop at the limit.** The page after the one that completed the budget
|
||||
is not requested — `pages` is a generator and this loop returns out of it.
|
||||
- **Stop at the page budget.** A predicate that rejects everything must not
|
||||
turn a bounded read into a walk of the whole tracker, so a filtered read
|
||||
may scan at most `PAGE_SLACK` times the pages `limit` would need if every
|
||||
payload counted. Hitting that with an unfilled budget is a warning, not a
|
||||
silent short answer: the caller asked for N and is told it got fewer."""
|
||||
if limit < 1:
|
||||
die("--limit must be 1 or more, got %d" % limit)
|
||||
ms_id, ms_title = (None, None)
|
||||
if milestone is not None:
|
||||
ms_id, ms_title = resolve_milestone(login, base, milestone)
|
||||
@@ -206,10 +244,25 @@ def list_issues(login, base, state="open", labels=(), query=None,
|
||||
endpoint = "%s/issues?%s" % (base, urllib.parse.urlencode(params))
|
||||
|
||||
per_page = min(limit, 50)
|
||||
got = paginate(login, endpoint, limit=per_page,
|
||||
max_pages=max(1, -(-limit // per_page)))
|
||||
got = [p for p in got if matches(p, ms_id, labels)]
|
||||
return got[:limit], ms_title
|
||||
ideal = max(1, -(-limit // per_page))
|
||||
budget = ideal if keep is None else ideal * PAGE_SLACK
|
||||
|
||||
got, kept, seen_pages, last_full = [], 0, 0, False
|
||||
for batch in pages(login, endpoint, limit=per_page, max_pages=budget):
|
||||
seen_pages += 1
|
||||
last_full = len(batch) == per_page
|
||||
for p in batch:
|
||||
if not matches(p, ms_id, labels):
|
||||
continue
|
||||
got.append(p)
|
||||
if keep is None or keep(p):
|
||||
kept += 1
|
||||
if kept >= limit:
|
||||
return got, ms_title
|
||||
if keep is not None and seen_pages >= budget and last_full:
|
||||
warn("scanned %d page(s) and stopped %d short of --limit %d — there may"
|
||||
" be more; narrow the filter or raise --limit" % (budget, limit - kept, limit))
|
||||
return got, ms_title
|
||||
|
||||
|
||||
def get_issue(login, base, number):
|
||||
|
||||
Reference in New Issue
Block a user