From 34dbd276bcb4a0680dc5772660d9a1e64c8b823f Mon Sep 17 00:00:00 2001 From: Leynos Date: Thu, 14 Aug 2025 12:24:07 +0100 Subject: [PATCH 1/6] Add start banner to code review output --- README.md | 9 +++++---- docs/vk-design.md | 11 ++++++----- src/main.rs | 10 +++++++++- src/summary.rs | 20 ++++++++++++++++++++ tests/cli.rs | 4 ++-- 5 files changed, 42 insertions(+), 12 deletions(-) diff --git a/README.md b/README.md index 2f8222d9..4496c086 100644 --- a/README.md +++ b/README.md @@ -29,10 +29,11 @@ sets the default repository when passing only a pull request number. The CLI provides two subcommands: -* `pr` — show unresolved pull request comments. A summary of files and comment - counts is printed first. When finished, `vk` prints an `end of code review` - banner. Pass file paths after the pull request to restrict output to those - paths. +* `pr` — show unresolved pull request comments. Output begins with a `code + review + ` banner followed by a summary of files and comment counts. When finished, `vk + ` prints an `end of code review` banner. Pass file paths after the pull + request to restrict output to those paths. * `issue` — read a GitHub issue (**to do**) `vk` loads default values for subcommands from configuration files and diff --git a/docs/vk-design.md b/docs/vk-design.md index c8221bfb..61c19a5e 100644 --- a/docs/vk-design.md +++ b/docs/vk-design.md @@ -21,7 +21,8 @@ even when multiple comments reference the same code. reducing clutter when multiple remarks target the same line. - **Error visibility**: Failures encountered while printing a thread are logged to stderr instead of being silently discarded. -- **Completion notice**: A final banner marks the *end of code review*. +- **Banners**: Output opens with a `code review` banner and ends with an + `end of code review` banner, framing the printed threads. ## Architecture @@ -33,10 +34,10 @@ The code centres on three printing helpers: 3. `write_thread` iterates over a thread and prints each comment body in turn. `run_pr` fetches the latest review banner from each reviewer and all unresolved -threads. The reviews are printed after the summary and before individual -threads. Errors from `print_thread` are surfaced via logging. Once all threads -have been printed, a final banner reading `end of code review` confirms -completion. +threads. After printing a `code review` banner and a summary, the reviews are +printed before individual threads. Errors from `print_thread` are surfaced via +logging. Once all threads have been printed, a final banner reading +`end of code review` confirms completion. ### CLI arguments diff --git a/src/main.rs b/src/main.rs index 3581ebdb..b0ceecde 100644 --- a/src/main.rs +++ b/src/main.rs @@ -33,7 +33,9 @@ pub use review_threads::{ CommentConnection, PageInfo, ReviewComment, ReviewThread, User, fetch_review_threads, filter_threads_by_files, }; -pub use summary::{print_end_banner, print_summary, summarize_files, write_summary}; +pub use summary::{ + print_end_banner, print_start_banner, print_summary, summarize_files, write_summary, +}; use crate::cli_args::{GlobalArgs, IssueArgs, PrArgs}; use crate::printer::{print_reviews, write_thread}; @@ -179,6 +181,12 @@ async fn run_pr(args: PrArgs, global: &GlobalArgs) -> Result<(), VkError> { fetch_review_threads(&client, &repo, number).await?, &args.files, ); + if let Err(e) = print_start_banner() { + if is_broken_pipe_io(&e) { + return Ok(()); + } + error!("error printing start banner: {e}"); + } // Avoid fetching reviews when there are no unresolved threads. if threads.is_empty() { if args.files.is_empty() { diff --git a/src/summary.rs b/src/summary.rs index e0b7946a..c9f82b9a 100644 --- a/src/summary.rs +++ b/src/summary.rs @@ -85,6 +85,26 @@ pub fn print_summary(summary: &[(String, usize)]) { } } +/// Print a banner marking the start of code review output. +/// +/// # Errors +/// +/// Returns an error if writing to stdout fails. +/// +/// # Examples +/// +/// ``` +/// use vk::summary::print_start_banner; +/// print_start_banner().unwrap(); +/// ``` +pub fn print_start_banner() -> std::io::Result<()> { + writeln!( + std::io::stdout().lock(), + "========== code review ==========" + )?; + Ok(()) +} + /// Print a closing banner once all review threads have been displayed. /// /// # Errors diff --git a/tests/cli.rs b/tests/cli.rs index e57533a1..6a012570 100644 --- a/tests/cli.rs +++ b/tests/cli.rs @@ -42,11 +42,11 @@ fn create_empty_review_handler() #[rstest] #[case( Vec::new(), - "No unresolved comments.\n========== end of code review ==========\n" + "========== code review ==========\nNo unresolved comments.\n========== end of code review ==========\n" )] #[case( vec!["no_such_file.rs"], - "No unresolved comments for the specified files.\n========== end of code review ==========\n", + "========== code review ==========\nNo unresolved comments for the specified files.\n========== end of code review ==========\n", )] #[tokio::test] async fn pr_empty_state( From 976a352964f68f3e5438c0dbfc92379d355ea88b Mon Sep 17 00:00:00 2001 From: Leynos Date: Thu, 14 Aug 2025 13:23:45 +0100 Subject: [PATCH 2/6] Refactor banner printing and extend coverage --- README.md | 9 +++---- src/main.rs | 34 +++++++++++++----------- src/summary.rs | 71 +++++++++++++++++++++++++++++++++++++++++++------- tests/cli.rs | 60 ++++++++++++++++++++++++++++++++++++++++++ 4 files changed, 144 insertions(+), 30 deletions(-) diff --git a/README.md b/README.md index 4496c086..4acc57ef 100644 --- a/README.md +++ b/README.md @@ -29,11 +29,10 @@ sets the default repository when passing only a pull request number. The CLI provides two subcommands: -* `pr` — show unresolved pull request comments. Output begins with a `code - review - ` banner followed by a summary of files and comment counts. When finished, `vk - ` prints an `end of code review` banner. Pass file paths after the pull - request to restrict output to those paths. +* `pr` — show unresolved pull request comments. Output begins with a + `code review` banner followed by a summary of files and comment counts. When + finished, `vk` prints an `end of code review` banner. Pass file paths after + the pull request to restrict output to those paths. * `issue` — read a GitHub issue (**to do**) `vk` loads default values for subcommands from configuration files and diff --git a/src/main.rs b/src/main.rs index b0ceecde..52b73e46 100644 --- a/src/main.rs +++ b/src/main.rs @@ -165,6 +165,19 @@ fn is_broken_pipe_io(err: &std::io::Error) -> bool { err.kind() == ErrorKind::BrokenPipe } +fn handle_banner(print: F, label: &str) -> bool +where + F: FnOnce() -> std::io::Result<()>, +{ + if let Err(e) = print() { + if is_broken_pipe_io(&e) { + return true; + } + error!("error printing {label} banner: {e}"); + } + false +} + async fn run_pr(args: PrArgs, global: &GlobalArgs) -> Result<(), VkError> { let reference = args.reference.as_deref().ok_or(VkError::InvalidRef)?; let (repo, number) = parse_pr_reference(reference, global.repo.as_deref())?; @@ -181,11 +194,8 @@ async fn run_pr(args: PrArgs, global: &GlobalArgs) -> Result<(), VkError> { fetch_review_threads(&client, &repo, number).await?, &args.files, ); - if let Err(e) = print_start_banner() { - if is_broken_pipe_io(&e) { - return Ok(()); - } - error!("error printing start banner: {e}"); + if handle_banner(print_start_banner, "start") { + return Ok(()); } // Avoid fetching reviews when there are no unresolved threads. if threads.is_empty() { @@ -195,11 +205,8 @@ async fn run_pr(args: PrArgs, global: &GlobalArgs) -> Result<(), VkError> { println!("No unresolved comments for the specified files."); } // Preserve the end-of-review banner for consumers that parse it. - if let Err(e) = print_end_banner() { - if is_broken_pipe_io(&e) { - return Ok(()); - } - error!("error printing end banner: {e}"); + if handle_banner(print_end_banner, "end") { + return Ok(()); } return Ok(()); } @@ -228,11 +235,8 @@ async fn run_pr(args: PrArgs, global: &GlobalArgs) -> Result<(), VkError> { } } - if let Err(e) = print_end_banner() { - if is_broken_pipe_io(&e) { - return Ok(()); - } - error!("error printing end banner: {e}"); + if handle_banner(print_end_banner, "end") { + return Ok(()); } Ok(()) } diff --git a/src/summary.rs b/src/summary.rs index c9f82b9a..77b3669f 100644 --- a/src/summary.rs +++ b/src/summary.rs @@ -85,6 +85,27 @@ pub fn print_summary(summary: &[(String, usize)]) { } } +fn write_banner(mut out: W, text: &str) -> std::io::Result<()> { + writeln!(out, "{text}") +} + +/// Write a banner marking the start of code review output to any writer. +/// +/// # Errors +/// +/// Returns an error if writing to the provided writer fails. +/// +/// # Examples +/// +/// ``` +/// use vk::summary::write_start_banner; +/// let mut out = Vec::new(); +/// write_start_banner(&mut out).unwrap(); +/// ``` +pub fn write_start_banner(out: W) -> std::io::Result<()> { + write_banner(out, "========== code review ==========") +} + /// Print a banner marking the start of code review output. /// /// # Errors @@ -98,11 +119,25 @@ pub fn print_summary(summary: &[(String, usize)]) { /// print_start_banner().unwrap(); /// ``` pub fn print_start_banner() -> std::io::Result<()> { - writeln!( - std::io::stdout().lock(), - "========== code review ==========" - )?; - Ok(()) + write_start_banner(std::io::stdout().lock()) +} + +/// Write a closing banner once all review threads have been displayed to any +/// writer. +/// +/// # Errors +/// +/// Returns an error if writing to the provided writer fails. +/// +/// # Examples +/// +/// ``` +/// use vk::summary::write_end_banner; +/// let mut out = Vec::new(); +/// write_end_banner(&mut out).unwrap(); +/// ``` +pub fn write_end_banner(out: W) -> std::io::Result<()> { + write_banner(out, "========== end of code review ==========") } /// Print a closing banner once all review threads have been displayed. @@ -118,11 +153,7 @@ pub fn print_start_banner() -> std::io::Result<()> { /// print_end_banner().unwrap(); /// ``` pub fn print_end_banner() -> std::io::Result<()> { - writeln!( - std::io::stdout().lock(), - "========== end of code review ==========" - )?; - Ok(()) + write_end_banner(std::io::stdout().lock()) } #[cfg(test)] @@ -191,4 +222,24 @@ mod tests { write_summary(&mut buf, &summary).expect("write summary"); assert!(buf.is_empty()); } + + #[test] + fn write_start_banner_propagates_io_errors() { + use std::io::{self, Write}; + + struct ErrorWriter; + impl Write for ErrorWriter { + fn write(&mut self, _buf: &[u8]) -> io::Result { + Err(io::Error::other("Simulated stdout write error")) + } + + fn flush(&mut self) -> io::Result<()> { + Ok(()) + } + } + + let mut writer = ErrorWriter; + let err = write_start_banner(&mut writer).expect_err("expect error"); + assert_eq!(err.to_string(), "Simulated stdout write error"); + } } diff --git a/tests/cli.rs b/tests/cli.rs index 6a012570..c2e34cac 100644 --- a/tests/cli.rs +++ b/tests/cli.rs @@ -11,6 +11,8 @@ use rstest::rstest; use serde_json::json; use std::process::Command; +use predicates::prelude::*; + mod utils; use utils::start_mitm; @@ -75,3 +77,61 @@ async fn pr_empty_state( shutdown.shutdown().await; } + +#[tokio::test] +async fn pr_outputs_banner_when_threads_present() { + let (addr, handler, shutdown) = start_mitm().await.expect("start server"); + let threads_body = json!({ + "data": {"repository": {"pullRequest": {"reviewThreads": { + "nodes": [{ + "id": "t1", + "isResolved": false, + "comments": { + "nodes": [{ + "body": "Looks good", + "diffHunk": "@@ -1 +1 @@\n-old\n+new\n", + "originalPosition": null, + "position": null, + "path": "file.rs", + "url": "http://example.com", + "author": {"login": "alice"} + }], + "pageInfo": {"hasNextPage": false, "endCursor": null} + } + }], + "pageInfo": {"hasNextPage": false, "endCursor": null} + }}}} + }) + .to_string(); + let reviews_body = json!({ + "data": {"repository": {"pullRequest": {"reviews": { + "nodes": [], + "pageInfo": {"hasNextPage": false, "endCursor": null} + }}}} + }) + .to_string(); + let mut responses = vec![threads_body, reviews_body].into_iter(); + *handler.lock().expect("lock handler") = Box::new(move |_req| { + let body = responses.next().expect("response"); + Response::builder() + .status(StatusCode::OK) + .header("Content-Type", "application/json") + .body(Full::from(body)) + .expect("build response") + }); + + tokio::task::spawn_blocking(move || { + let mut cmd = Command::cargo_bin("vk").expect("binary"); + cmd.env("GITHUB_GRAPHQL_URL", format!("http://{addr}")) + .env("GITHUB_TOKEN", "dummy") + .args(["pr", "https://github.com/leynos/shared-actions/pull/42"]); + cmd.assert().success().stdout( + predicate::str::starts_with("========== code review ==========\n") + .and(predicate::str::contains("Looks good")), + ); + }) + .await + .expect("spawn blocking"); + + shutdown.shutdown().await; +} From aed3d5128ae528c35430b31db15f147126c46b70 Mon Sep 17 00:00:00 2001 From: Leynos Date: Thu, 14 Aug 2025 17:13:17 +0100 Subject: [PATCH 3/6] Refine banner docs and tests --- README.md | 2 +- docs/vk-design.md | 11 ++++++----- src/main.rs | 19 ++++++++++++++++--- src/summary.rs | 46 ++++++++++++++++++++++++++++++++++++---------- 4 files changed, 59 insertions(+), 19 deletions(-) diff --git a/README.md b/README.md index 4acc57ef..d8482d80 100644 --- a/README.md +++ b/README.md @@ -30,7 +30,7 @@ sets the default repository when passing only a pull request number. The CLI provides two subcommands: * `pr` — show unresolved pull request comments. Output begins with a - `code review` banner followed by a summary of files and comment counts. When + `code review` banner, followed by a summary of files and comment counts. When finished, `vk` prints an `end of code review` banner. Pass file paths after the pull request to restrict output to those paths. * `issue` — read a GitHub issue (**to do**) diff --git a/docs/vk-design.md b/docs/vk-design.md index 61c19a5e..d29016cb 100644 --- a/docs/vk-design.md +++ b/docs/vk-design.md @@ -33,11 +33,12 @@ The code centres on three printing helpers: 2. `write_comment` includes the diff for the first comment in a thread. 3. `write_thread` iterates over a thread and prints each comment body in turn. -`run_pr` fetches the latest review banner from each reviewer and all unresolved +`run_pr` fetches the latest review from each reviewer and all unresolved threads. After printing a `code review` banner and a summary, the reviews are -printed before individual threads. Errors from `print_thread` are surfaced via -logging. Once all threads have been printed, a final banner reading -`end of code review` confirms completion. +printed before individual threads. Broken pipe errors terminate output early; +other errors from `print_thread` and banner printing are surfaced via logging. +Once all threads have been printed, a final banner reading `end of code review` +confirms completion. ### CLI arguments @@ -121,7 +122,7 @@ classDiagram ReviewComment "0..*" --> "0..1" User : author CommentConnection "1" --> "1" PageInfo : pageInfo - class ReviewThreadsService <> { + class ReviewThreadsService { +fetchReviewThreads(client: GraphQLClient, repo: String, number: Int): [ReviewThread!]! } ``` diff --git a/src/main.rs b/src/main.rs index 52b73e46..50dcd32d 100644 --- a/src/main.rs +++ b/src/main.rs @@ -6,9 +6,9 @@ //! `review_threads` for fetching review data, `issues` for issue retrieval, //! `summary` for summarising comments, and `config` for configuration //! management. When a thread has multiple comments on the same diff, the diff -//! is shown only once. After all comments are printed, the tool displays an -//! `end of code review` banner so calling processes know the output has -//! finished. +//! is shown only once. Output is framed by a `code review` banner at the start +//! and an `end of code review` banner at the end so calling processes can +//! reliably detect boundaries. pub mod api; mod boxed; @@ -455,4 +455,17 @@ mod tests { assert!(out.contains("\u{25B6} hello")); assert!(!out.contains("bye")); } + + #[test] + fn handle_banner_returns_true_on_broken_pipe() { + let broken_pipe = + || -> std::io::Result<()> { Err(std::io::Error::from(std::io::ErrorKind::BrokenPipe)) }; + assert!(super::handle_banner(broken_pipe, "start")); + } + + #[test] + fn handle_banner_logs_and_returns_false_on_other_errors() { + let other_err = || -> std::io::Result<()> { Err(std::io::Error::other("boom")) }; + assert!(!super::handle_banner(other_err, "end")); + } } diff --git a/src/summary.rs b/src/summary.rs index 77b3669f..20d6473f 100644 --- a/src/summary.rs +++ b/src/summary.rs @@ -1,13 +1,19 @@ -//! Utilities for generating and printing review summaries. +//! Utilities for generating and printing review summaries and banners. //! //! Functions in this module collate review comments by file path and render a -//! human-readable summary to any writer or directly to stdout. +//! human-readable summary to any writer or directly to stdout. Banner helpers +//! frame output with start and end markers. use std::collections::BTreeMap; use std::io::{ErrorKind, Write}; use crate::review_threads::ReviewThread; +/// Banner printed at the start of a code review. +pub const START_BANNER: &str = "========== code review =========="; +/// Banner printed at the end of a code review. +pub const END_BANNER: &str = "========== end of code review =========="; + /// Produce a count of comments per file path. /// /// # Examples @@ -56,8 +62,8 @@ pub fn summarize_files(threads: &[ReviewThread]) -> Vec<(String, usize)> { /// }; /// let summary = summarize_files(&[thread]); /// let mut out = Vec::new(); -/// write_summary(&mut out, &summary).unwrap(); -/// assert!(String::from_utf8(out).unwrap().contains("a.rs: 1 comment")); +/// write_summary(&mut out, &summary).expect("write summary"); +/// assert!(String::from_utf8(out).expect("utf8").contains("a.rs: 1 comment")); /// ``` pub fn write_summary( mut out: W, @@ -100,10 +106,10 @@ fn write_banner(mut out: W, text: &str) -> std::io::Result<()> { /// ``` /// use vk::summary::write_start_banner; /// let mut out = Vec::new(); -/// write_start_banner(&mut out).unwrap(); +/// write_start_banner(&mut out).expect("write start banner"); /// ``` pub fn write_start_banner(out: W) -> std::io::Result<()> { - write_banner(out, "========== code review ==========") + write_banner(out, START_BANNER) } /// Print a banner marking the start of code review output. @@ -116,7 +122,7 @@ pub fn write_start_banner(out: W) -> std::io::Result<()> { /// /// ``` /// use vk::summary::print_start_banner; -/// print_start_banner().unwrap(); +/// print_start_banner().expect("print start banner"); /// ``` pub fn print_start_banner() -> std::io::Result<()> { write_start_banner(std::io::stdout().lock()) @@ -134,10 +140,10 @@ pub fn print_start_banner() -> std::io::Result<()> { /// ``` /// use vk::summary::write_end_banner; /// let mut out = Vec::new(); -/// write_end_banner(&mut out).unwrap(); +/// write_end_banner(&mut out).expect("write end banner"); /// ``` pub fn write_end_banner(out: W) -> std::io::Result<()> { - write_banner(out, "========== end of code review ==========") + write_banner(out, END_BANNER) } /// Print a closing banner once all review threads have been displayed. @@ -150,7 +156,7 @@ pub fn write_end_banner(out: W) -> std::io::Result<()> { /// /// ``` /// use vk::summary::print_end_banner; -/// print_end_banner().unwrap(); +/// print_end_banner().expect("print end banner"); /// ``` pub fn print_end_banner() -> std::io::Result<()> { write_end_banner(std::io::stdout().lock()) @@ -242,4 +248,24 @@ mod tests { let err = write_start_banner(&mut writer).expect_err("expect error"); assert_eq!(err.to_string(), "Simulated stdout write error"); } + + #[test] + fn write_end_banner_propagates_io_errors() { + use std::io::{self, Write}; + + struct ErrorWriter; + impl Write for ErrorWriter { + fn write(&mut self, _buf: &[u8]) -> io::Result { + Err(io::Error::other("Simulated stdout write error")) + } + + fn flush(&mut self) -> io::Result<()> { + Ok(()) + } + } + + let mut writer = ErrorWriter; + let err = write_end_banner(&mut writer).expect_err("expect error"); + assert_eq!(err.to_string(), "Simulated stdout write error"); + } } From ac3b07f2b886e2fb3f2c554a477465130c7b405b Mon Sep 17 00:00:00 2001 From: Leynos Date: Thu, 14 Aug 2025 17:48:39 +0100 Subject: [PATCH 4/6] Emit start banner before network calls Call the start banner helper before network requests so output begins immediately. Handle broken pipes in the empty review case, add a unit test for banner content, and switch README bullets to dashes. --- README.md | 4 ++-- src/main.rs | 22 +++++++++++++++------- src/summary.rs | 10 ++++++++++ 3 files changed, 27 insertions(+), 9 deletions(-) diff --git a/README.md b/README.md index d8482d80..c025f19f 100644 --- a/README.md +++ b/README.md @@ -29,11 +29,11 @@ sets the default repository when passing only a pull request number. The CLI provides two subcommands: -* `pr` — show unresolved pull request comments. Output begins with a +- `pr` — show unresolved pull request comments. Output begins with a `code review` banner, followed by a summary of files and comment counts. When finished, `vk` prints an `end of code review` banner. Pass file paths after the pull request to restrict output to those paths. -* `issue` — read a GitHub issue (**to do**) +- `issue` — read a GitHub issue (**to do**) `vk` loads default values for subcommands from configuration files and environment variables. When these defaults omit the required `reference` field, diff --git a/src/main.rs b/src/main.rs index 50dcd32d..dad1b7e6 100644 --- a/src/main.rs +++ b/src/main.rs @@ -46,7 +46,7 @@ use log::{error, warn}; use regex::Regex; use serde::{Deserialize, Serialize}; use std::env; -use std::io::ErrorKind; +use std::io::{ErrorKind, Write}; use std::sync::LazyLock; use termimad::MadSkin; use thiserror::Error; @@ -189,20 +189,28 @@ async fn run_pr(args: PrArgs, global: &GlobalArgs) -> Result<(), VkError> { warn!("terminal locale is not UTF-8; emojis may not render correctly"); } + // Emit the start banner as the first line of output. + if handle_banner(print_start_banner, "start") { + return Ok(()); + } + let client = build_graphql_client(&token, global.transcript.as_ref())?; let threads = filter_threads_by_files( fetch_review_threads(&client, &repo, number).await?, &args.files, ); - if handle_banner(print_start_banner, "start") { - return Ok(()); - } // Avoid fetching reviews when there are no unresolved threads. if threads.is_empty() { - if args.files.is_empty() { - println!("No unresolved comments."); + let msg = if args.files.is_empty() { + "No unresolved comments." } else { - println!("No unresolved comments for the specified files."); + "No unresolved comments for the specified files." + }; + if let Err(e) = writeln!(std::io::stdout().lock(), "{msg}") { + if is_broken_pipe_io(&e) { + return Ok(()); + } + error!("error writing empty state: {e}"); } // Preserve the end-of-review banner for consumers that parse it. if handle_banner(print_end_banner, "end") { diff --git a/src/summary.rs b/src/summary.rs index 20d6473f..152983da 100644 --- a/src/summary.rs +++ b/src/summary.rs @@ -249,6 +249,16 @@ mod tests { assert_eq!(err.to_string(), "Simulated stdout write error"); } + #[test] + fn write_start_banner_outputs_text() { + let mut buf = Vec::new(); + write_start_banner(&mut buf).expect("write start banner"); + assert_eq!( + String::from_utf8(buf).expect("utf8"), + format!("{START_BANNER}\n"), + ); + } + #[test] fn write_end_banner_propagates_io_errors() { use std::io::{self, Write}; From 6f2fdfde04cdd7499de7583940070493ff4d5be5 Mon Sep 17 00:00:00 2001 From: Leynos Date: Thu, 14 Aug 2025 17:48:45 +0100 Subject: [PATCH 5/6] Clarify README start-banner formatting --- README.md | 8 ++++---- 1 file changed, 4 insertions(+), 4 deletions(-) diff --git a/README.md b/README.md index c025f19f..17133622 100644 --- a/README.md +++ b/README.md @@ -29,10 +29,10 @@ sets the default repository when passing only a pull request number. The CLI provides two subcommands: -- `pr` — show unresolved pull request comments. Output begins with a - `code review` banner, followed by a summary of files and comment counts. When - finished, `vk` prints an `end of code review` banner. Pass file paths after - the pull request to restrict output to those paths. +- `pr` — show unresolved pull request comments. It begins with a `code review` + banner, summarises files and comment counts, then prints an + `end of code review` banner. Pass file paths after the pull request to + restrict output to those paths. - `issue` — read a GitHub issue (**to do**) `vk` loads default values for subcommands from configuration files and From 8557cd9ba7b22734772a44c98f89abdb206794f2 Mon Sep 17 00:00:00 2001 From: Leynos Date: Thu, 14 Aug 2025 18:30:15 +0100 Subject: [PATCH 6/6] Refactor PR flow into helper functions --- src/main.rs | 91 ++++++++++++++++++++++++++++++++++---------------- src/summary.rs | 2 +- 2 files changed, 64 insertions(+), 29 deletions(-) diff --git a/src/main.rs b/src/main.rs index dad1b7e6..00fe69d5 100644 --- a/src/main.rs +++ b/src/main.rs @@ -8,7 +8,10 @@ //! management. When a thread has multiple comments on the same diff, the diff //! is shown only once. Output is framed by a `code review` banner at the start //! and an `end of code review` banner at the end so calling processes can -//! reliably detect boundaries. +//! reliably detect boundaries. The module re-exports banner helpers +//! [`print_start_banner`] and [`print_end_banner`] alongside summary utilities +//! [`print_summary`], [`summarize_files`], and [`write_summary`] so consumers can +//! reuse the framing and summarisation logic. pub mod api; mod boxed; @@ -39,8 +42,8 @@ pub use summary::{ use crate::cli_args::{GlobalArgs, IssueArgs, PrArgs}; use crate::printer::{print_reviews, write_thread}; -use crate::ref_parser::{parse_issue_reference, parse_pr_reference}; -use crate::reviews::{fetch_reviews, latest_reviews}; +use crate::ref_parser::{RepoInfo, parse_issue_reference, parse_pr_reference}; +use crate::reviews::{PullRequestReview, fetch_reviews, latest_reviews}; use clap::{Parser, Subcommand}; use log::{error, warn}; use regex::Regex; @@ -178,7 +181,11 @@ where false } -async fn run_pr(args: PrArgs, global: &GlobalArgs) -> Result<(), VkError> { +/// Prepare PR context, validate environment and print the start banner. +fn setup_pr_output( + args: &PrArgs, + global: &GlobalArgs, +) -> Result<(RepoInfo, u64, GraphQLClient), VkError> { let reference = args.reference.as_deref().ok_or(VkError::InvalidRef)?; let (repo, number) = parse_pr_reference(reference, global.repo.as_deref())?; let token = env::var("GITHUB_TOKEN").unwrap_or_default(); @@ -188,38 +195,45 @@ async fn run_pr(args: PrArgs, global: &GlobalArgs) -> Result<(), VkError> { if !locale_is_utf8() { warn!("terminal locale is not UTF-8; emojis may not render correctly"); } - - // Emit the start banner as the first line of output. if handle_banner(print_start_banner, "start") { - return Ok(()); + return Err(VkError::Io(Box::new(std::io::Error::new( + ErrorKind::BrokenPipe, + "broken pipe", + )))); } - let client = build_graphql_client(&token, global.transcript.as_ref())?; - let threads = filter_threads_by_files( - fetch_review_threads(&client, &repo, number).await?, - &args.files, - ); - // Avoid fetching reviews when there are no unresolved threads. - if threads.is_empty() { - let msg = if args.files.is_empty() { - "No unresolved comments." - } else { - "No unresolved comments for the specified files." - }; - if let Err(e) = writeln!(std::io::stdout().lock(), "{msg}") { - if is_broken_pipe_io(&e) { - return Ok(()); - } - error!("error writing empty state: {e}"); - } - // Preserve the end-of-review banner for consumers that parse it. - if handle_banner(print_end_banner, "end") { + Ok((repo, number, client)) +} + +/// Print an appropriate message when no threads match and append the end banner. +#[allow( + clippy::unnecessary_wraps, + reason = "returns Result for interface symmetry" +)] +fn handle_empty_threads(files: &[String]) -> Result<(), VkError> { + let msg = if files.is_empty() { + "No unresolved comments." + } else { + "No unresolved comments for the specified files." + }; + if let Err(e) = writeln!(std::io::stdout().lock(), "{msg}") { + if is_broken_pipe_io(&e) { return Ok(()); } + error!("error writing empty state: {e}"); + } + if handle_banner(print_end_banner, "end") { return Ok(()); } - let reviews = fetch_reviews(&client, &repo, number).await?; + Ok(()) +} +/// Render the summary, reviews and threads, then print the closing banner. +#[allow(clippy::unnecessary_wraps, reason = "future error cases may emerge")] +fn generate_pr_output( + threads: Vec, + reviews: Vec, +) -> Result<(), VkError> { let summary = summarize_files(&threads); print_summary(&summary); @@ -249,6 +263,27 @@ async fn run_pr(args: PrArgs, global: &GlobalArgs) -> Result<(), VkError> { Ok(()) } +async fn run_pr(args: PrArgs, global: &GlobalArgs) -> Result<(), VkError> { + let (repo, number, client) = match setup_pr_output(&args, global) { + Ok(v) => v, + Err(VkError::Io(e)) if e.kind() == ErrorKind::BrokenPipe => return Ok(()), + Err(e) => return Err(e), + }; + + let threads = filter_threads_by_files( + fetch_review_threads(&client, &repo, number).await?, + &args.files, + ); + + if threads.is_empty() { + handle_empty_threads(&args.files)?; + return Ok(()); + } + + let reviews = fetch_reviews(&client, &repo, number).await?; + generate_pr_output(threads, reviews) +} + async fn run_issue(args: IssueArgs, global: &GlobalArgs) -> Result<(), VkError> { let reference = args.reference.as_deref().ok_or(VkError::InvalidRef)?; let (repo, number) = parse_issue_reference(reference, global.repo.as_deref())?; diff --git a/src/summary.rs b/src/summary.rs index 152983da..71e3a5c5 100644 --- a/src/summary.rs +++ b/src/summary.rs @@ -250,7 +250,7 @@ mod tests { } #[test] - fn write_start_banner_outputs_text() { + fn write_start_banner_outputs_exact_text() { let mut buf = Vec::new(); write_start_banner(&mut buf).expect("write start banner"); assert_eq!(