From 219d85a15d5f964f4df6d43ea96b0c8eccb862da Mon Sep 17 00:00:00 2001 From: damocles Date: Sun, 1 Nov 2026 01:54:55 +0100 Subject: [PATCH 1/2] forge cli: show comment ids and resolved state on inline review comments, --only unresolved, and list them in pr timeline --- docs/tools/forge-cli.md | 6 + docs/tools/forge.md | 4 +- hive-forge/src/verbs/pr_reviews.rs | 252 ++++++++++++++++++++++++----- hive-forge/src/verbs/timeline.rs | 117 +++++++++++++- 4 files changed, 335 insertions(+), 44 deletions(-) diff --git a/docs/tools/forge-cli.md b/docs/tools/forge-cli.md index 8e6610f69..114c146e9 100644 --- a/docs/tools/forge-cli.md +++ b/docs/tools/forge-cli.md @@ -607,6 +607,12 @@ 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 + + Possible values: + - `unresolved`: + Neither resolved nor outdated + diff --git a/docs/tools/forge.md b/docs/tools/forge.md index ef4f71b67..26a6dec74 100644 --- a/docs/tools/forge.md +++ b/docs/tools/forge.md @@ -77,7 +77,9 @@ 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; inline comments included per review +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 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 hive-forge pr reviews 42 --comment -m "msg" # submit COMMENT review diff --git a/hive-forge/src/verbs/pr_reviews.rs b/hive-forge/src/verbs/pr_reviews.rs index b00b4ffbe..859727cb9 100644 --- a/hive-forge/src/verbs/pr_reviews.rs +++ b/hive-forge/src/verbs/pr_reviews.rs @@ -1,6 +1,8 @@ //! `pr reviews ` — list reviews, or submit one via `--approve` / //! `--request-changes` / `--comment`. +use std::fmt::Write as _; + use anyhow::{Result, bail}; use clap::Args as ClapArgs; use forgejo_api::structs::CreatePullReviewOptions; @@ -30,6 +32,20 @@ pub struct Args { /// --request-changes, or --comment). #[arg(long, short = 'm')] 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. + #[arg(long, value_enum, conflicts_with_all = ["approve", "request_changes", "comment"])] + only: Option, +} + +/// Which inline comments `--only` keeps. +#[derive(Clone, Copy, PartialEq, Eq, clap::ValueEnum)] +enum Only { + /// Neither resolved nor outdated. + Unresolved, } pub fn run(client: &Client, args: Args) -> Result<()> { @@ -49,7 +65,7 @@ pub fn run(client: &Client, args: Args) -> Result<()> { if args.body.is_some() { bail!("--body requires one of --approve / --request-changes / --comment"); } - list_reviews(client, args.number) + list_reviews(client, args.number, args.only) } } @@ -77,7 +93,7 @@ fn submit_review(client: &Client, number: u64, event: &str, body: Option /// Fetch inline diff comments for a single review, serialized back to /// the API's JSON shape. Returns an empty vec on any error (missing /// review, network failure) so callers can degrade gracefully. -fn fetch_inline_comments(client: &Client, pr: u64, review_id: i64) -> Vec { +pub(crate) fn fetch_inline_comments(client: &Client, pr: u64, review_id: i64) -> Vec { let Ok((owner, name)) = client.owner_repo() else { return Vec::new(); }; @@ -95,7 +111,7 @@ fn fetch_inline_comments(client: &Client, pr: u64, review_id: i64) -> Vec } /// List all reviews for a PR, dispatching to the appropriate output mode. -fn list_reviews(client: &Client, number: u64) -> Result<()> { +fn list_reviews(client: &Client, number: u64, only: Option) -> Result<()> { let (owner, name) = client.owner_repo()?; let idx = index(number)?; let reviews = all_pages(|page, size| { @@ -110,35 +126,114 @@ fn list_reviews(client: &Client, number: u64) -> Result<()> { let reviews = serde_json::to_value(reviews)?; let reviews = reviews.as_array().cloned().unwrap_or_default(); if client.json_mode() { - list_reviews_json(client, number, &reviews) + list_reviews_json(client, number, &reviews, only) } else { - list_reviews_text(client, number, &reviews); + list_reviews_text(client, number, &reviews, only); Ok(()) } } +/// Where an inline comment sits: `path:line`, or the bare `path` when +/// Forgejo dropped the position because the line left the diff. Control +/// characters in the path become spaces so one cannot shape the output. +pub(crate) fn comment_location(c: &Value) -> String { + let path: String = c + .get("path") + .and_then(Value::as_str) + .unwrap_or("?") + .chars() + .map(|ch| if ch.is_control() { ' ' } else { ch }) + .collect(); + match c.get("position").and_then(Value::as_u64).filter(|&l| l > 0) { + Some(line) => format!("{path}:{line}"), + None => path, + } +} + +/// 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 { + c.get("position") + .and_then(Value::as_u64) + .is_none_or(|l| l == 0) +} + +/// Login of whoever resolved the conversation, if anyone did. +fn resolved_by(c: &Value) -> Option<&str> { + c.get("resolver") + .and_then(|r| r.get("login")) + .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) +} + +/// One inline comment as text: `[#id path:line] body`, continuation lines +/// indented, `(resolved by X)` / `(outdated)` 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); + let body = c + .get("body") + .and_then(Value::as_str) + .unwrap_or("") + .trim() + .replace('\n', "\n "); + let mut out = format!("[#{id} {}] {body}", comment_location(c)); + if let Some(who) = resolved_by(c) { + write!(out, " (resolved by {who})").ok(); + } + if is_outdated(c) { + out.push_str(" (outdated)"); + } + out +} + +/// The comments to show for one review: all of them, or with `--only` just +/// the matching ones. +fn visible(comments: Vec, only: Option) -> Vec { + match only { + Some(Only::Unresolved) => comments.into_iter().filter(is_open).collect(), + None => comments, + } +} + /// JSON output: one object per review, with an inline `comments` array. -fn list_reviews_json(client: &Client, number: u64, reviews: &[Value]) -> Result<()> { +/// With `--only`, only reviews that still have an open comment. +fn list_reviews_json( + client: &Client, + number: u64, + reviews: &[Value], + only: Option, +) -> Result<()> { let trimmed: Vec = reviews .iter() - .map(|r| { + .filter_map(|r| { let id = r.get("id").and_then(Value::as_i64).unwrap_or(0); - let inline: Vec = if id > 0 { - fetch_inline_comments(client, number, id) - .iter() - .map(|c| { - json!({ - "id": c.get("id"), - "path": c.get("path"), - "line": c.get("position"), - "body": c.get("body"), - }) - }) - .collect() + let inline = if id > 0 { + visible(fetch_inline_comments(client, number, id), only) } else { vec![] }; - json!({ + if only.is_some() && inline.is_empty() { + return None; + } + let inline: Vec = inline + .iter() + .map(|c| { + json!({ + "id": c.get("id"), + "path": c.get("path"), + "line": c.get("position"), + "body": c.get("body"), + "resolved_by": resolved_by(c), + "outdated": is_outdated(c), + }) + }) + .collect(); + Some(json!({ "id": r.get("id"), "state": r.get("state"), // forgejo's staleness bits: `stale` = head moved since the @@ -152,21 +247,28 @@ fn list_reviews_json(client: &Client, number: u64, reviews: &[Value]) -> Result< "body": r.get("body"), "comments_count": r.get("comments_count"), "comments": inline, - }) + })) }) .collect(); print_json(&Value::Array(trimmed)) } /// Human-readable output: Markdown-style heading per review, inline -/// comments as `[path:line] body` (line omitted for PR-level comments). -fn list_reviews_text(client: &Client, number: u64, reviews: &[Value]) { - if reviews.is_empty() { - println!("(no reviews)"); - return; - } +/// comments via [`render_inline_comment`]. With `--only`, only reviews +/// that still have an open comment, and only those comments. +fn list_reviews_text(client: &Client, number: u64, reviews: &[Value], only: Option) { + let mut shown = false; for r in reviews { let id = r.get("id").and_then(Value::as_i64).unwrap_or(0); + let inline = if id > 0 { + visible(fetch_inline_comments(client, number, id), only) + } else { + vec![] + }; + if only.is_some() && inline.is_empty() { + continue; + } + shown = true; let user = r .get("user") .and_then(|u| u.get("login")) @@ -186,23 +288,91 @@ fn list_reviews_text(client: &Client, number: u64, reviews: &[Value]) { "" }; println!("### review by {user} ({state}){flag}"); - if !body.is_empty() { + if !body.is_empty() && only.is_none() { println!("{body}"); } - if id > 0 { - for c in &fetch_inline_comments(client, number, id) { - let path = c.get("path").and_then(Value::as_str).unwrap_or("?"); - let cbody = c.get("body").and_then(Value::as_str).unwrap_or("").trim(); - // PR-level comments have no position; omit `:line` when - // absent (also true for a comment whose anchored line has - // since fallen out of the diff — forgejo drops `position` - // in that case too, same display fallback). - match c.get("position").and_then(Value::as_u64) { - Some(line) => println!(" [{path}:{line}] {cbody}"), - None => println!(" [{path}] {cbody}"), - } - } + for c in &inline { + println!(" {}", render_inline_comment(c)); } println!(); } + if !shown { + println!( + "{}", + if only.is_some() { + "(no matching inline comments)" + } else { + "(no reviews)" + } + ); + } +} + +#[cfg(test)] +mod tests { + use super::*; + + fn comment(id: i64, position: u64, resolver: Option<&str>) -> Value { + json!({ + "id": id, + "path": "src/a.rs", + "position": position, + "body": "off by one", + "resolver": resolver.map(|login| json!({"login": login})), + }) + } + + #[test] + fn location_is_path_line_or_bare_path() { + assert_eq!(comment_location(&comment(1, 42, None)), "src/a.rs:42"); + assert_eq!(comment_location(&comment(1, 0, None)), "src/a.rs"); + } + + #[test] + fn location_neutralises_control_characters() { + let c = json!({"path": "a\nb\tc.rs", "position": 3}); + assert_eq!(comment_location(&c), "a b c.rs:3"); + } + + #[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))); + } + + #[test] + fn unresolved_filter_keeps_only_open_comments() { + let all = vec![ + comment(1, 42, None), + comment(2, 42, Some("mara")), + comment(3, 0, None), + ]; + 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, None)), vec![1, 2, 3]); + } + + #[test] + fn render_shows_id_location_body_and_state_tags() { + assert_eq!( + render_inline_comment(&comment(7, 42, None)), + "[#7 src/a.rs:42] off by one" + ); + assert_eq!( + render_inline_comment(&comment(7, 42, Some("mara"))), + "[#7 src/a.rs:42] off by one (resolved by mara)" + ); + assert_eq!( + render_inline_comment(&comment(7, 0, None)), + "[#7 src/a.rs] off by one (outdated)" + ); + } + + #[test] + fn render_indents_continuation_lines() { + let c = json!({"id": 1, "path": "f.rs", "position": 2, "body": "one\ntwo"}); + assert_eq!(render_inline_comment(&c), "[#1 f.rs:2] one\n two"); + } } diff --git a/hive-forge/src/verbs/timeline.rs b/hive-forge/src/verbs/timeline.rs index 8c65d4c6f..ddd2782ad 100644 --- a/hive-forge/src/verbs/timeline.rs +++ b/hive-forge/src/verbs/timeline.rs @@ -32,6 +32,7 @@ use serde_json::{Value, json}; use time::format_description::well_known::Rfc3339; use crate::client::{Client, index}; +use crate::verbs::pr_reviews::{fetch_inline_comments, render_inline_comment}; use crate::verbs::{MAX_LIMIT, NullableVec, PAGE_SIZE, clamp_limit, parse_rfc3339, print_json}; #[derive(ClapArgs)] @@ -85,7 +86,10 @@ pub fn run(client: &Client, args: Args) -> Result<()> { }; // Serialize back to the API's JSON shape so the per-type render // arms (and their tests) keep working on plain `Value`s. - let v = serde_json::to_value(&events)?; + let mut v = serde_json::to_value(&events)?; + if let Some(arr) = v.as_array_mut() { + attach_inline_comments(client, args.number, arr); + } if client.json_mode() { return print_json(&json!({ "events": v, @@ -285,6 +289,52 @@ fn fetch_since( Ok((events, more)) } +/// Attach the inline (line) comments to the review events of a PR timeline. +/// +/// The timeline row for a line comment carries no file or line, and the +/// `review` row none of its comments, so both are looked up per review: each +/// `code` row gets its comment under `inline` (path, position, resolver), and +/// a `review` row with no `code` row of its own in this window gets all its +/// comments under `inline_comments`, so they show up whichever way the forge +/// lists them, and never twice. One fetch per distinct review; a failed one +/// leaves the rows as they were. +fn attach_inline_comments(client: &Client, number: u64, events: &mut [Value]) { + let review_id = |ev: &Value| { + ev.get("review_id") + .and_then(Value::as_i64) + .filter(|&id| id > 0) + }; + let kind = |ev: &Value| ev.get("type").and_then(Value::as_str).map(str::to_owned); + let with_code: std::collections::HashSet = events + .iter() + .filter(|ev| kind(ev).as_deref() == Some("code")) + .filter_map(review_id) + .collect(); + let mut fetched: std::collections::HashMap> = std::collections::HashMap::new(); + for ev in events.iter_mut() { + let (Some(rid), Some(k)) = (review_id(ev), kind(ev)) else { + continue; + }; + if k != "code" && k != "review" { + continue; + } + let comments = fetched + .entry(rid) + .or_insert_with(|| fetch_inline_comments(client, number, rid)); + if k == "code" { + let id = ev.get("id").and_then(Value::as_i64); + if let Some(c) = comments + .iter() + .find(|c| c.get("id").and_then(Value::as_i64) == id) + { + ev["inline"] = c.clone(); + } + } else if !with_code.contains(&rid) && !comments.is_empty() { + ev["inline_comments"] = Value::Array(comments.clone()); + } + } +} + /// Render one timeline event as a single `**actor @ ts**: summary` /// line. Comment rows inline their full body; structured event types /// (label, assignees, close, etc.) get a one-line human summary @@ -439,7 +489,24 @@ fn format_event(ev: &Value) -> String { format!("{verb} #{number} ({title})") } "changed_target_branch" => "changed target branch".to_owned(), - "review" => "submitted a review".to_owned(), + "review" => { + let mut out = "submitted a review".to_owned(); + let inline = ev.get("inline_comments").and_then(Value::as_array); + for c in inline.into_iter().flatten() { + out.push_str("\n "); + out.push_str(&render_inline_comment(c)); + } + out + } + // A line comment: the comment text, led by where it sits and its + // id when `attach_inline_comments` could look that up. + "code" => { + let body = ev.get("body").and_then(Value::as_str).unwrap_or(""); + match ev.get("inline") { + Some(c) => format!("commented on a line\n {}", render_inline_comment(c)), + None => format!("commented on a line: {body}"), + } + } "lock" => "locked the conversation".to_owned(), "unlock" => "unlocked the conversation".to_owned(), // Future / unknown types: surface the raw label so we don't @@ -699,4 +766,50 @@ mod tests { }); assert_eq!(format_event(&ev), "**? @ 2026-05-31T12:00:00Z**: closed"); } + + #[test] + fn code_event_without_lookup_shows_the_bare_body() { + let ev = serde_json::json!({ + "type": "code", "user": {"login": "argus"}, + "created_at": "2026-11-01T01:00:00Z", "body": "off by one", + }); + assert_eq!( + format_event(&ev), + "**argus @ 2026-11-01T01:00:00Z**: commented on a line: off by one" + ); + } + + #[test] + fn code_event_with_lookup_shows_id_location_and_state() { + let ev = serde_json::json!({ + "type": "code", "user": {"login": "argus"}, + "created_at": "2026-11-01T01:00:00Z", "body": "off by one", + "inline": {"id": 9, "path": "src/a.rs", "position": 42, "body": "off by one", + "resolver": {"login": "mara"}}, + }); + assert_eq!( + format_event(&ev), + "**argus @ 2026-11-01T01:00:00Z**: commented on a line\n [#9 src/a.rs:42] off by one (resolved by mara)" + ); + } + + #[test] + fn review_event_lists_its_inline_comments() { + let ev = serde_json::json!({ + "type": "review", "user": {"login": "argus"}, + "created_at": "2026-11-01T01:00:00Z", + "inline_comments": [{"id": 9, "path": "src/a.rs", "position": 42, "body": "x"}], + }); + assert_eq!( + format_event(&ev), + "**argus @ 2026-11-01T01:00:00Z**: submitted a review\n [#9 src/a.rs:42] x" + ); + // Without the lookup it stays the one-line summary. + let bare = serde_json::json!({"type": "review", "user": {"login": "argus"}, + "created_at": "2026-11-01T01:00:00Z"}); + assert_eq!( + format_event(&bare), + "**argus @ 2026-11-01T01:00:00Z**: submitted a review" + ); + } } From 1dc18d41d4db57a1f13d9e9d32ed878d18fe720b Mon Sep 17 00:00:00 2001 From: damocles Date: Sun, 1 Nov 2026 02:08:37 +0100 Subject: [PATCH 2/2] forge cli: unresolved filter drops only resolved comments, position-less ones stay labelled --- docs/tools/forge-cli.md | 4 +-- docs/tools/forge.md | 4 +-- hive-forge/src/verbs/pr_reviews.rs | 52 +++++++++++++++++------------- 3 files changed, 34 insertions(+), 26 deletions(-) diff --git a/docs/tools/forge-cli.md b/docs/tools/forge-cli.md index 114c146e9..12b0b192b 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 26a6dec74..41680e78e 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 859727cb9..84c1b862a 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)" ); }