diff --git a/docs/tools/forge-cli.md b/docs/tools/forge-cli.md index 114c146e..12b0b192 100644 --- a/docs/tools/forge-cli.md +++ b/docs/tools/forge-cli.md @@ -607,11 +607,11 @@ List a PR's reviews, or submit one * `--request-changes` — Request changes on the PR (submit a `REQUEST_CHANGES` review) * `--comment` — Leave a comment review (submit a COMMENT review) * `-m`, `--body ` — Optional body / message for the review (used with --approve, --request-changes, or --comment) -* `--only ` — When listing: narrow the inline comments shown (and leave out the reviews with none left). `unresolved` = still open on the current head, that is neither resolved nor outdated: what a re-review has to check +* `--only ` — When listing: narrow the inline comments shown (and leave out the reviews with none left). `unresolved` keeps every comment nobody has resolved yet: what a re-review has to check Possible values: - `unresolved`: - Neither resolved nor outdated + Not resolved yet. Comments without a position stay in: one might sit on a line a push moved or on the whole file, and hiding it could hide a live finding diff --git a/docs/tools/forge.md b/docs/tools/forge.md index 26a6dec7..41680e78 100644 --- a/docs/tools/forge.md +++ b/docs/tools/forge.md @@ -77,8 +77,8 @@ hive-forge pr edit 42 --base main # retarget a stacked PR onto main hive-forge pr 42 # PR metadata as JSON hive-forge pr create --title "..." --head my-branch --push # also `git push forge my-branch` hive-forge pr create --title "..." --head my-branch --label area/ops # repeatable, same as issue create -hive-forge pr reviews 42 # list reviews; each inline comment as `[#id path:line] text`, tagged (resolved by X) / (outdated) -hive-forge pr reviews 42 --only unresolved # only inline comments still open on the current head +hive-forge pr reviews 42 # list reviews; each inline comment as `[#id path:line] text`, tagged (resolved by X) / (no position) +hive-forge pr reviews 42 --only unresolved # every inline comment nobody has resolved yet hive-forge pr timeline 42 # review rows list their line comments (id, path:line, state) too hive-forge pr reviews 42 --approve # submit APPROVED review hive-forge pr reviews 42 --request-changes -m "msg" # submit REQUEST_CHANGES review diff --git a/hive-forge/src/verbs/pr_reviews.rs b/hive-forge/src/verbs/pr_reviews.rs index 859727cb..84c1b862 100644 --- a/hive-forge/src/verbs/pr_reviews.rs +++ b/hive-forge/src/verbs/pr_reviews.rs @@ -34,9 +34,8 @@ pub struct Args { body: Option, /// When listing: narrow the inline comments shown (and leave out the - /// reviews with none left). `unresolved` = still open on the current - /// head, that is neither resolved nor outdated: what a re-review has to - /// check. + /// reviews with none left). `unresolved` keeps every comment nobody has + /// resolved yet: what a re-review has to check. #[arg(long, value_enum, conflicts_with_all = ["approve", "request_changes", "comment"])] only: Option, } @@ -44,7 +43,9 @@ pub struct Args { /// Which inline comments `--only` keeps. #[derive(Clone, Copy, PartialEq, Eq, clap::ValueEnum)] enum Only { - /// Neither resolved nor outdated. + /// Not resolved yet. Comments without a position stay in: one might sit on + /// a line a push moved or on the whole file, and hiding it could hide a + /// live finding. Unresolved, } @@ -150,9 +151,10 @@ pub(crate) fn comment_location(c: &Value) -> String { } } -/// An inline comment whose anchored line is no longer in the diff (a push -/// moved it): Forgejo drops `position` then. -fn is_outdated(c: &Value) -> bool { +/// An inline comment with no line position. Forgejo drops `position` when a +/// push moved the anchored line out of the diff, but a file-level comment or +/// one on a removed line may lack it too, so this is a hint, not a verdict. +fn has_no_position(c: &Value) -> bool { c.get("position") .and_then(Value::as_u64) .is_none_or(|l| l == 0) @@ -165,13 +167,15 @@ fn resolved_by(c: &Value) -> Option<&str> { .and_then(Value::as_str) } -/// Still needs an answer on the current head: neither resolved nor outdated. -fn is_open(c: &Value) -> bool { - resolved_by(c).is_none() && !is_outdated(c) +/// Nobody has resolved the conversation yet. Position plays no part: a +/// comment without one may be a live finding, and a filter built for a +/// re-review gate must not hide it. +fn is_unresolved(c: &Value) -> bool { + resolved_by(c).is_none() } /// One inline comment as text: `[#id path:line] body`, continuation lines -/// indented, `(resolved by X)` / `(outdated)` after the body. Shared by +/// indented, `(resolved by X)` / `(no position: ...)` after the body. Shared by /// `pr reviews` and `pr timeline`. pub(crate) fn render_inline_comment(c: &Value) -> String { let id = c.get("id").and_then(Value::as_i64).unwrap_or(0); @@ -185,8 +189,8 @@ pub(crate) fn render_inline_comment(c: &Value) -> String { if let Some(who) = resolved_by(c) { write!(out, " (resolved by {who})").ok(); } - if is_outdated(c) { - out.push_str(" (outdated)"); + if has_no_position(c) { + out.push_str(" (no position: outdated or file-level)"); } out } @@ -195,7 +199,7 @@ pub(crate) fn render_inline_comment(c: &Value) -> String { /// the matching ones. fn visible(comments: Vec, only: Option) -> Vec { match only { - Some(Only::Unresolved) => comments.into_iter().filter(is_open).collect(), + Some(Only::Unresolved) => comments.into_iter().filter(is_unresolved).collect(), None => comments, } } @@ -229,7 +233,7 @@ fn list_reviews_json( "line": c.get("position"), "body": c.get("body"), "resolved_by": resolved_by(c), - "outdated": is_outdated(c), + "no_position": has_no_position(c), }) }) .collect(); @@ -335,14 +339,15 @@ mod tests { } #[test] - fn open_means_neither_resolved_nor_outdated() { - assert!(is_open(&comment(1, 42, None))); - assert!(!is_open(&comment(1, 42, Some("mara")))); - assert!(!is_open(&comment(1, 0, None))); + fn unresolved_means_only_that_nobody_resolved_it() { + assert!(is_unresolved(&comment(1, 42, None))); + assert!(!is_unresolved(&comment(1, 42, Some("mara")))); + // No position does not hide a comment: it may be a live finding. + assert!(is_unresolved(&comment(1, 0, None))); } #[test] - fn unresolved_filter_keeps_only_open_comments() { + fn unresolved_filter_drops_only_resolved_comments() { let all = vec![ comment(1, 42, None), comment(2, 42, Some("mara")), @@ -350,7 +355,10 @@ mod tests { ]; let ids = |v: Vec| -> Vec { v.iter().filter_map(|c| c["id"].as_i64()).collect() }; - assert_eq!(ids(visible(all.clone(), Some(Only::Unresolved))), vec![1]); + assert_eq!( + ids(visible(all.clone(), Some(Only::Unresolved))), + vec![1, 3] + ); assert_eq!(ids(visible(all, None)), vec![1, 2, 3]); } @@ -366,7 +374,7 @@ mod tests { ); assert_eq!( render_inline_comment(&comment(7, 0, None)), - "[#7 src/a.rs] off by one (outdated)" + "[#7 src/a.rs] off by one (no position: outdated or file-level)" ); }