diff --git a/docs/tools/forge.md b/docs/tools/forge.md index 41680e78..3030945e 100644 --- a/docs/tools/forge.md +++ b/docs/tools/forge.md @@ -77,7 +77,7 @@ 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) / (no position) +hive-forge pr reviews 42 # list reviews; each inline comment as `[#id path:line] text`, tagged (by X) when its poster is not the reviewer / (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 diff --git a/hive-forge/src/verbs/pr_reviews.rs b/hive-forge/src/verbs/pr_reviews.rs index 84c1b862..07d1219c 100644 --- a/hive-forge/src/verbs/pr_reviews.rs +++ b/hive-forge/src/verbs/pr_reviews.rs @@ -167,6 +167,13 @@ fn resolved_by(c: &Value) -> Option<&str> { .and_then(Value::as_str) } +/// Login of whoever posted the comment, if the forge says. +fn posted_by(c: &Value) -> Option<&str> { + c.get("user") + .and_then(|u| u.get("login")) + .and_then(Value::as_str) +} + /// 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. @@ -175,9 +182,14 @@ fn is_unresolved(c: &Value) -> bool { } /// One inline comment as text: `[#id path:line] body`, continuation lines -/// 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 { +/// indented, `(by X)` / `(resolved by X)` / `(no position: ...)` after the body. +/// Shared by `pr reviews` and `pr timeline`. +/// +/// A reply is stored in the review of the comment it answers, so under +/// `reviewer`'s heading it can be someone else's: `(by X)` names the poster +/// when it differs from `reviewer`. `pr timeline` shows who commented on its +/// own and passes `None`. +pub(crate) fn render_inline_comment(c: &Value, reviewer: Option<&str>) -> String { let id = c.get("id").and_then(Value::as_i64).unwrap_or(0); let body = c .get("body") @@ -186,6 +198,11 @@ pub(crate) fn render_inline_comment(c: &Value) -> String { .trim() .replace('\n', "\n "); let mut out = format!("[#{id} {}] {body}", comment_location(c)); + if let (Some(poster), Some(reviewer)) = (posted_by(c), reviewer) + && poster != reviewer + { + write!(out, " (by {poster})").ok(); + } if let Some(who) = resolved_by(c) { write!(out, " (resolved by {who})").ok(); } @@ -232,6 +249,7 @@ fn list_reviews_json( "path": c.get("path"), "line": c.get("position"), "body": c.get("body"), + "user": posted_by(c), "resolved_by": resolved_by(c), "no_position": has_no_position(c), }) @@ -296,7 +314,7 @@ fn list_reviews_text(client: &Client, number: u64, reviews: &[Value], only: Opti println!("{body}"); } for c in &inline { - println!(" {}", render_inline_comment(c)); + println!(" {}", render_inline_comment(c, Some(user))); } println!(); } @@ -365,15 +383,15 @@ mod tests { #[test] fn render_shows_id_location_body_and_state_tags() { assert_eq!( - render_inline_comment(&comment(7, 42, None)), + render_inline_comment(&comment(7, 42, None), None), "[#7 src/a.rs:42] off by one" ); assert_eq!( - render_inline_comment(&comment(7, 42, Some("mara"))), + render_inline_comment(&comment(7, 42, Some("mara")), None), "[#7 src/a.rs:42] off by one (resolved by mara)" ); assert_eq!( - render_inline_comment(&comment(7, 0, None)), + render_inline_comment(&comment(7, 0, None), None), "[#7 src/a.rs] off by one (no position: outdated or file-level)" ); } @@ -381,6 +399,30 @@ mod tests { #[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"); + assert_eq!(render_inline_comment(&c, None), "[#1 f.rs:2] one\n two"); + } + + #[test] + fn a_reply_by_someone_else_names_its_poster_under_the_reviewers_heading() { + let mut c = comment(7, 42, None); + c["user"] = json!({"login": "damocles"}); + assert_eq!( + render_inline_comment(&c, Some("mara")), + "[#7 src/a.rs:42] off by one (by damocles)" + ); + // The reviewer's own comment, a comment with no poster, and a caller + // that shows the poster elsewhere get no tag. + assert_eq!( + render_inline_comment(&c, Some("damocles")), + "[#7 src/a.rs:42] off by one" + ); + assert_eq!( + render_inline_comment(&comment(7, 42, None), Some("mara")), + "[#7 src/a.rs:42] off by one" + ); + assert_eq!( + render_inline_comment(&c, None), + "[#7 src/a.rs:42] off by one" + ); } } diff --git a/hive-forge/src/verbs/timeline.rs b/hive-forge/src/verbs/timeline.rs index ddd2782a..7843f548 100644 --- a/hive-forge/src/verbs/timeline.rs +++ b/hive-forge/src/verbs/timeline.rs @@ -494,7 +494,7 @@ fn format_event(ev: &Value) -> String { 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.push_str(&render_inline_comment(c, None)); } out } @@ -503,7 +503,7 @@ fn format_event(ev: &Value) -> String { "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)), + Some(c) => format!("commented on a line\n {}", render_inline_comment(c, None)), None => format!("commented on a line: {body}"), } }