diff --git a/README.md b/README.md index 2f8222d9..17133622 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. 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. -* `issue` — read a GitHub issue (**to do**) +- `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 environment variables. When these defaults omit the required `reference` field, diff --git a/docs/vk-design.md b/docs/vk-design.md index c8221bfb..d29016cb 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 @@ -32,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 -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. +`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. 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 @@ -120,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 3581ebdb..00fe69d5 100644 --- a/src/main.rs +++ b/src/main.rs @@ -6,9 +6,12 @@ //! `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. 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; @@ -33,18 +36,20 @@ 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}; -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; 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; @@ -163,7 +168,24 @@ fn is_broken_pipe_io(err: &std::io::Error) -> bool { err.kind() == ErrorKind::BrokenPipe } -async fn run_pr(args: PrArgs, global: &GlobalArgs) -> Result<(), VkError> { +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 +} + +/// 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(); @@ -173,30 +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"); } - + if handle_banner(print_start_banner, "start") { + 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() { - if args.files.is_empty() { - println!("No unresolved comments."); - } else { - 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}"); + 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); @@ -220,15 +257,33 @@ 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(()) } +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())?; @@ -443,4 +498,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 e0b7946a..71e3a5c5 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, @@ -85,6 +91,61 @@ 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).expect("write start banner"); +/// ``` +pub fn write_start_banner(out: W) -> std::io::Result<()> { + write_banner(out, START_BANNER) +} + +/// 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().expect("print start banner"); +/// ``` +pub fn print_start_banner() -> std::io::Result<()> { + 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).expect("write end banner"); +/// ``` +pub fn write_end_banner(out: W) -> std::io::Result<()> { + write_banner(out, END_BANNER) +} + /// Print a closing banner once all review threads have been displayed. /// /// # Errors @@ -95,14 +156,10 @@ pub fn print_summary(summary: &[(String, usize)]) { /// /// ``` /// 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<()> { - writeln!( - std::io::stdout().lock(), - "========== end of code review ==========" - )?; - Ok(()) + write_end_banner(std::io::stdout().lock()) } #[cfg(test)] @@ -171,4 +228,54 @@ 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"); + } + + #[test] + fn write_start_banner_outputs_exact_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}; + + 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"); + } } diff --git a/tests/cli.rs b/tests/cli.rs index e57533a1..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; @@ -42,11 +44,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( @@ -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; +}