hive-forge: comments --since --limit returns N, not at most 49
The issue-comments endpoint takes no page or limit, so `comments --since` cut its window client-side from a whole-list response. The leftover over-fetch-by-one request (`.page_size(limit + 1)`) was a no-op and the `--limit` clamp to MAX_LIMIT (49) guarded a truncation check that no longer needs one. Drop both: `--since T --limit N` now returns N comments. `comments --json` still emits `since_limit_clamped`, always `false`. `timeline` is untouched; its endpoint pages and its clamp stays.
This commit is contained in:
atlas
parent
1d56a74aad
commit
3d81266483
4 changed files with 54 additions and 50 deletions
|
|
@ -242,7 +242,7 @@ List comments on the issue
|
|||
|
||||
###### **Options:**
|
||||
|
||||
* `--limit <LIMIT>` — Number of comments from the start of the thread (oldest-first), or (with `--since`) the most this call returns — capped at [`crate::verbs::MAX_LIMIT`] in the latter case. An explicit opt-in: with neither this nor `--tail` given, the default is the newest 10 (see `--tail`). Mutually exclusive with `--tail`
|
||||
* `--limit <LIMIT>` — Number of comments from the start of the thread (oldest-first), or (with `--since`) the most this call returns. An explicit opt-in: with neither this nor `--tail` given, the default is the newest 10 (see `--tail`). Mutually exclusive with `--tail`
|
||||
* `--tail <TAIL>` — Return the last `N` comments (chronological) — the most recent activity. Defaults to `N` = 10 when you pass neither `--limit` nor `--tail`. Mutually exclusive with `--limit`/`--since`
|
||||
* `--since <SINCE>` — Only show comments at or after this RFC3339 timestamp (same format this verb's own output prints). Mutually exclusive with `--tail`
|
||||
* `--show-reactions` — Fetch + display each shown comment's reaction summary. Costs one extra request per comment shown — opt-in, not the default
|
||||
|
|
@ -704,7 +704,7 @@ List comments on the PR
|
|||
|
||||
###### **Options:**
|
||||
|
||||
* `--limit <LIMIT>` — Number of comments from the start of the thread (oldest-first), or (with `--since`) the most this call returns — capped at [`crate::verbs::MAX_LIMIT`] in the latter case. An explicit opt-in: with neither this nor `--tail` given, the default is the newest 10 (see `--tail`). Mutually exclusive with `--tail`
|
||||
* `--limit <LIMIT>` — Number of comments from the start of the thread (oldest-first), or (with `--since`) the most this call returns. An explicit opt-in: with neither this nor `--tail` given, the default is the newest 10 (see `--tail`). Mutually exclusive with `--tail`
|
||||
* `--tail <TAIL>` — Return the last `N` comments (chronological) — the most recent activity. Defaults to `N` = 10 when you pass neither `--limit` nor `--tail`. Mutually exclusive with `--limit`/`--since`
|
||||
* `--since <SINCE>` — Only show comments at or after this RFC3339 timestamp (same format this verb's own output prints). Mutually exclusive with `--tail`
|
||||
* `--show-reactions` — Fetch + display each shown comment's reaction summary. Costs one extra request per comment shown — opt-in, not the default
|
||||
|
|
|
|||
|
|
@ -16,17 +16,18 @@
|
|||
//! - `--since <RFC3339>` filters to comments at or after that
|
||||
//! timestamp instead of a head/tail window — a cursor: feed the
|
||||
//! last-seen row's own `created_at` back in next time. Mutually
|
||||
//! exclusive with `--tail`; `--limit` clamps like `timeline`'s own
|
||||
//! does (see `crate::verbs::MAX_LIMIT`).
|
||||
//! exclusive with `--tail`; `--limit` windows the filtered list as
|
||||
//! given, uncapped.
|
||||
//!
|
||||
//! Review *bodies* on PRs are always merged in too (`pulls/<n>/reviews`
|
||||
//! isn't the issues/comments thread), tagged `[review: STATE]`, with a
|
||||
//! `(N line comment(s) — see 'pr reviews')` pointer for inline comments.
|
||||
//!
|
||||
//! `--json` output is an object (`{"comments": [...], "more_before": N,
|
||||
//! "more_after": N, "since_more": bool}`), not a bare array, so a
|
||||
//! script can read the truncation info too. `--show-reactions` adds a
|
||||
//! per-comment reaction summary — see that flag's own doc for the cost.
|
||||
//! "more_after": N, "since_more": bool, "since_limit_clamped": false}`),
|
||||
//! not a bare array, so a script can read the truncation info too.
|
||||
//! `--show-reactions` adds a per-comment reaction summary — see that
|
||||
//! flag's own doc for the cost.
|
||||
|
||||
use anyhow::Result;
|
||||
use clap::Args as ClapArgs;
|
||||
|
|
@ -37,8 +38,7 @@ use time::OffsetDateTime;
|
|||
use crate::client::{Client, index};
|
||||
use crate::notify;
|
||||
use crate::verbs::{
|
||||
MAX_LIMIT, all_pages, clamp_limit, comment_reactions, parse_rfc3339, print_json,
|
||||
reaction_summary, rfc3339,
|
||||
all_pages, comment_reactions, parse_rfc3339, print_json, reaction_summary, rfc3339,
|
||||
};
|
||||
|
||||
#[derive(ClapArgs)]
|
||||
|
|
@ -46,8 +46,7 @@ pub struct Args {
|
|||
/// Issue or PR number.
|
||||
pub(crate) number: u64,
|
||||
/// Number of comments from the start of the thread (oldest-first),
|
||||
/// or (with `--since`) the most this call returns — capped at
|
||||
/// [`crate::verbs::MAX_LIMIT`] in the latter case. An explicit
|
||||
/// or (with `--since`) the most this call returns. An explicit
|
||||
/// opt-in: with neither this nor `--tail` given, the default is the
|
||||
/// newest 10 (see `--tail`). Mutually exclusive with `--tail`.
|
||||
#[arg(long, conflicts_with = "tail")]
|
||||
|
|
@ -76,24 +75,22 @@ pub fn run(client: &Client, args: Args) -> Result<()> {
|
|||
// starts at the thread's beginning); a `--since` window is a boolean
|
||||
// "there's more" (no total exists for a since-filtered query) rather
|
||||
// than an exact count.
|
||||
let (thread, more_before, more_after, since_more, since_clamped) =
|
||||
if let Some(since_str) = &args.since {
|
||||
let since = parse_rfc3339(since_str)?;
|
||||
let (limit, clamped) = clamp_limit(args.limit.unwrap_or(10));
|
||||
let (thread, more) = fetch_since(client, args.number, since, limit)?;
|
||||
(thread, 0, 0, more, clamped)
|
||||
} else if let Some(limit) = args.limit {
|
||||
let (thread, more_after) = head(fetch_thread(client, args.number)?, limit);
|
||||
(thread, 0, more_after, false, false)
|
||||
} else {
|
||||
// Default (no `--limit`/`--tail`/`--since`): same window as
|
||||
// an explicit `--tail 10` — the newest activity, not the
|
||||
// oldest. `args.tail` still wins if the agent passed a
|
||||
// different count explicitly.
|
||||
let n = args.tail.unwrap_or(10);
|
||||
let (thread, more_before) = tail(fetch_thread(client, args.number)?, n);
|
||||
(thread, more_before, 0, false, false)
|
||||
};
|
||||
let (thread, more_before, more_after, since_more) = if let Some(since_str) = &args.since {
|
||||
let since = parse_rfc3339(since_str)?;
|
||||
let (thread, more) = fetch_since(client, args.number, since, args.limit.unwrap_or(10))?;
|
||||
(thread, 0, 0, more)
|
||||
} else if let Some(limit) = args.limit {
|
||||
let (thread, more_after) = head(fetch_thread(client, args.number)?, limit);
|
||||
(thread, 0, more_after, false)
|
||||
} else {
|
||||
// Default (no `--limit`/`--tail`/`--since`): same window as
|
||||
// an explicit `--tail 10` — the newest activity, not the
|
||||
// oldest. `args.tail` still wins if the agent passed a
|
||||
// different count explicitly.
|
||||
let n = args.tail.unwrap_or(10);
|
||||
let (thread, more_before) = tail(fetch_thread(client, args.number)?, n);
|
||||
(thread, more_before, 0, false)
|
||||
};
|
||||
// Merge in PR review bodies (empty for issues — degrades to a
|
||||
// no-op) so review feedback isn't silently dropped.
|
||||
let mut comments = merge_chronological(thread, fetch_review_bodies(client, args.number));
|
||||
|
|
@ -126,7 +123,7 @@ pub fn run(client: &Client, args: Args) -> Result<()> {
|
|||
"more_before": more_before,
|
||||
"more_after": more_after,
|
||||
"since_more": since_more,
|
||||
"since_limit_clamped": since_clamped,
|
||||
"since_limit_clamped": false,
|
||||
}))
|
||||
} else {
|
||||
for c in &comments {
|
||||
|
|
@ -163,12 +160,6 @@ pub fn run(client: &Client, args: Args) -> Result<()> {
|
|||
}
|
||||
println!();
|
||||
}
|
||||
if since_clamped {
|
||||
println!(
|
||||
"(--limit {} is above the {MAX_LIMIT} cap --since can reliably detect truncation at — clamped)",
|
||||
args.limit.unwrap_or(10)
|
||||
);
|
||||
}
|
||||
if since_more {
|
||||
println!(
|
||||
"(more comments since this timestamp not shown — raise --limit or bump --since; exact count not available)"
|
||||
|
|
@ -388,12 +379,10 @@ fn tail(mut thread: Vec<Value>, n: usize) -> (Vec<Value>, usize) {
|
|||
(thread, more_before)
|
||||
}
|
||||
|
||||
/// Fetch comments at or after `since`, capped at `limit` (already
|
||||
/// clamped to [`crate::verbs::MAX_LIMIT`] by the caller). No total exists
|
||||
/// for a since-filtered query — Forgejo's API reports none — so this
|
||||
/// uses the same over-fetch-by-one trick `timeline`'s `--limit` does:
|
||||
/// ask for `limit + 1`, and a full extra row means there's more. Returns
|
||||
/// `(comments, more)`, `more` a boolean rather than an exact count.
|
||||
/// Comments at or after `since`, the first `limit` of them. The endpoint
|
||||
/// takes no page or limit, so the filtered list arrives whole and `more`
|
||||
/// (whether any follow the window) is read off its length. A boolean
|
||||
/// rather than a count: a since-filtered window has no total to report.
|
||||
fn fetch_since(
|
||||
client: &Client,
|
||||
number: u64,
|
||||
|
|
@ -405,17 +394,12 @@ fn fetch_since(
|
|||
since: Some(since),
|
||||
..Default::default()
|
||||
};
|
||||
let fetch_limit = limit.saturating_add(1);
|
||||
let (_, comments) = client
|
||||
.api()
|
||||
.issue_get_comments(owner, name, index(number)?, query)
|
||||
.page_size(u32::try_from(fetch_limit).unwrap_or(u32::MAX))
|
||||
.send()?;
|
||||
let mut values = to_values(comments)?;
|
||||
let limit = usize::try_from(limit).unwrap_or(usize::MAX);
|
||||
let more = values.len() > limit;
|
||||
values.truncate(limit);
|
||||
Ok((values, more))
|
||||
let (values, more_after) = head(to_values(comments)?, limit);
|
||||
Ok((values, more_after > 0))
|
||||
}
|
||||
|
||||
#[cfg(test)]
|
||||
|
|
|
|||
|
|
@ -101,7 +101,7 @@ pub(crate) fn parse_rfc3339(s: &str) -> Result<OffsetDateTime> {
|
|||
|
||||
/// Forgejo's per-page cap, shared by every listing verb that over-fetches
|
||||
/// by one to detect truncation without an exact total (`timeline`'s
|
||||
/// `--limit`, `comments`' `--since`). The API silently clamps a requested
|
||||
/// `--limit`). The API silently clamps a requested
|
||||
/// page size to this value, so it's pinned explicitly rather than left as
|
||||
/// a hidden default downstream math could drift out of sync with.
|
||||
pub(crate) const PAGE_SIZE: u64 = 50;
|
||||
|
|
|
|||
|
|
@ -137,3 +137,23 @@ fn comments_tail_returns_the_last_n() {
|
|||
assert!(!stdout.contains(&format!("comment {}", THREAD_LEN - 4)));
|
||||
assert!(stdout.contains(&format!("({} earlier comment(s)", THREAD_LEN - 3)));
|
||||
}
|
||||
|
||||
#[test]
|
||||
fn comments_since_limit_above_a_page_returns_exactly_limit() {
|
||||
let stdout = hive_forge(
|
||||
"since-limit",
|
||||
&[
|
||||
"--json",
|
||||
"comments",
|
||||
NUMBER,
|
||||
"--since",
|
||||
"2026-10-09T00:00:00Z",
|
||||
"--limit",
|
||||
"55",
|
||||
],
|
||||
);
|
||||
let out: serde_json::Value = serde_json::from_str(&stdout).expect("comments --json output");
|
||||
assert_eq!(out["comments"].as_array().map(Vec::len), Some(55));
|
||||
assert_eq!(out["since_more"], true);
|
||||
assert_eq!(out["since_limit_clamped"], false);
|
||||
}
|
||||
|
|
|
|||
Loading…
Reference in a new issue