From 29431f3fbc530d87bcda74488e3e137f707e6bfa Mon Sep 17 00:00:00 2001 From: Leynos Date: Sun, 20 Jul 2025 12:31:28 +0100 Subject: [PATCH 1/3] Add comment summary for vk pr --- README.md | 29 ++++++++++++++---------- src/main.rs | 63 +++++++++++++++++++++++++++++++++++++++++++++++++++++ 2 files changed, 81 insertions(+), 11 deletions(-) diff --git a/README.md b/README.md index 6166aa97..22a9ae53 100644 --- a/README.md +++ b/README.md @@ -1,8 +1,13 @@ # View Komments (vk) -`vk` stands for **View Komments** because `vc` was already taken back in the 1970s and no one argues with a greybeard. This command line tool fetches unresolved GitHub code review comments for a pull request and displays them with colourful terminal markdown using [Termimad](https://crates.io/crates/termimad). +`vk` stands for **View Komments** because `vc` was already taken back in the +1970s and no one argues with a greybeard. This command line tool fetches +unresolved GitHub code review comments for a pull request and displays them with +colourful terminal markdown using +[Termimad](https://crates.io/crates/termimad). -This tool is intended for use by AI coding agents such as Aider, OpenAI Codex or Claude Code (without implying association with any of these companies). +This tool is intended for use by AI coding agents such as Aider, OpenAI Codex or +Claude Code (without implying association with any of these companies). ## Usage @@ -16,19 +21,20 @@ sets the default repository when passing only a pull request number. The CLI provides two subcommands: -* `pr` - show unresolved pull request comments (existing behaviour) +* `pr` - show unresolved pull request comments. A summary of files and comment + counts is printed first. * `issue` - read a GitHub issue (**to do**) -If you pass just a pull request number, `vk` tries to work out which -repository you meant. It first examines `.git/FETCH_HEAD` for a GitHub remote -URL and, if found, extracts the `owner/repo` from it. As the Codex agent does not put the +If you pass just a pull request number, `vk` tries to work out which repository +you meant. It first examines `.git/FETCH_HEAD` for a GitHub remote URL and, if +found, extracts the `owner/repo` from it. As the Codex agent does not put the upstream URL in `.git/config`, we must obtain this from `FETCH_HEAD` for now. Failing that, it falls back to the configured repository (`--repo` or `VK_REPO`). Set this value to `owner/repo` with or without a `.git` suffix. If neither source is available, `vk` will refuse to run with only a number. -`vk` uses the GitHub GraphQL API. Set `GITHUB_TOKEN` to authenticate. If it's not -set you'll get a warning and anonymous requests may be rate limited. +`vk` uses the GitHub GraphQL API. Set `GITHUB_TOKEN` to authenticate. If it's +not set you'll get a warning and anonymous requests may be rate limited. The token only needs read access. Select the `public_repo` scope (or `repo` for private repositories). See [docs/GITHUB_TOKEN.md](docs/GITHUB_TOKEN.md) for a @@ -36,8 +42,8 @@ detailed guide to creating one. ## Example -``` -$ vk pr https://github.com/leynos/mxd/pull/31 +```bash +vk pr https://github.com/leynos/mxd/pull/31 ``` ## Troubleshooting @@ -56,4 +62,5 @@ cargo install --path . ## License -This project is licensed under the ISC License. See [LICENSE](LICENSE) for details. +This project is licensed under the ISC License. See +[LICENSE](LICENSE) for details. diff --git a/src/main.rs b/src/main.rs index d3dfe02e..456ac2a9 100644 --- a/src/main.rs +++ b/src/main.rs @@ -572,6 +572,17 @@ fn print_comment(skin: &MadSkin, comment: &ReviewComment) -> anyhow::Result<()> Ok(()) } +fn summarize_files(threads: &[ReviewThread]) -> Vec<(String, usize)> { + use std::collections::BTreeMap; + let mut counts: BTreeMap = BTreeMap::new(); + for t in threads { + for c in &t.comments.nodes { + *counts.entry(c.path.clone()).or_default() += 1; + } + } + counts.into_iter().collect() +} + fn build_headers(token: &str) -> HeaderMap { let mut headers = HeaderMap::new(); headers.insert(USER_AGENT, "vk".parse().unwrap()); @@ -601,6 +612,15 @@ async fn run_pr(args: PrArgs, repo: Option<&str>) -> Result<(), VkError> { return Ok(()); } + let summary = summarize_files(&threads); + if !summary.is_empty() { + println!("Summary:"); + for (path, count) in summary { + println!("{path}: {count}"); + } + println!(); + } + let skin = MadSkin::default(); for t in threads { for c in &t.comments.nodes { @@ -984,4 +1004,47 @@ mod tests { assert_eq!(repo.name, "qux"); assert_eq!(number, 8); } + + #[test] + fn summarize_files_counts_comments() { + fn comment(path: &str) -> ReviewComment { + ReviewComment { + body: String::new(), + diff_hunk: String::new(), + original_position: None, + position: None, + path: path.into(), + url: String::new(), + author: None, + } + } + + let threads = vec![ + ReviewThread { + id: String::new(), + is_resolved: false, + comments: CommentConnection { + nodes: vec![comment("a.rs"), comment("b.rs")], + page_info: PageInfo { + has_next_page: false, + end_cursor: None, + }, + }, + }, + ReviewThread { + id: String::new(), + is_resolved: false, + comments: CommentConnection { + nodes: vec![comment("a.rs")], + page_info: PageInfo { + has_next_page: false, + end_cursor: None, + }, + }, + }, + ]; + + let summary = summarize_files(&threads); + assert_eq!(summary, vec![("a.rs".into(), 2), ("b.rs".into(), 1)]); + } } From 1d0e624200a2dbaa6b877d10a92764b65e99528a Mon Sep 17 00:00:00 2001 From: Leynos Date: Sun, 20 Jul 2025 12:50:50 +0100 Subject: [PATCH 2/3] Refactor pr summary output --- src/main.rs | 46 +++++++++++++++++++++++++++++++++++++++------- 1 file changed, 39 insertions(+), 7 deletions(-) diff --git a/src/main.rs b/src/main.rs index 456ac2a9..dacd0fe5 100644 --- a/src/main.rs +++ b/src/main.rs @@ -583,6 +583,25 @@ fn summarize_files(threads: &[ReviewThread]) -> Vec<(String, usize)> { counts.into_iter().collect() } +fn write_summary( + mut out: W, + summary: &[(String, usize)], +) -> std::io::Result<()> { + if summary.is_empty() { + return Ok(()); + } + writeln!(out, "Summary:")?; + for (path, count) in summary { + writeln!(out, "{path}: {count}")?; + } + writeln!(out)?; + Ok(()) +} + +fn print_summary(summary: &[(String, usize)]) { + let _ = write_summary(std::io::stdout().lock(), summary); +} + fn build_headers(token: &str) -> HeaderMap { let mut headers = HeaderMap::new(); headers.insert(USER_AGENT, "vk".parse().unwrap()); @@ -613,13 +632,7 @@ async fn run_pr(args: PrArgs, repo: Option<&str>) -> Result<(), VkError> { } let summary = summarize_files(&threads); - if !summary.is_empty() { - println!("Summary:"); - for (path, count) in summary { - println!("{path}: {count}"); - } - println!(); - } + print_summary(&summary); let skin = MadSkin::default(); for t in threads { @@ -1047,4 +1060,23 @@ mod tests { let summary = summarize_files(&threads); assert_eq!(summary, vec![("a.rs".into(), 2), ("b.rs".into(), 1)]); } + + #[test] + fn write_summary_outputs_text() { + let summary = vec![("a.rs".into(), 2), ("b.rs".into(), 1)]; + let mut buf = Vec::new(); + write_summary(&mut buf, &summary).unwrap(); + let out = String::from_utf8(buf).unwrap(); + assert!(out.contains("Summary:")); + assert!(out.contains("a.rs: 2")); + assert!(out.contains("b.rs: 1")); + } + + #[test] + fn write_summary_handles_empty() { + let summary: Vec<(String, usize)> = Vec::new(); + let mut buf = Vec::new(); + write_summary(&mut buf, &summary).unwrap(); + assert!(buf.is_empty()); + } } From 7363e0019d2515ce76f3a7630f0575168f014c6c Mon Sep 17 00:00:00 2001 From: Leynos Date: Sun, 20 Jul 2025 13:02:51 +0100 Subject: [PATCH 3/3] Improve summary tests and docs --- Cargo.lock | 85 +++++++++++++++++++++++++++++++++++++++++++++++++++ Cargo.toml | 1 + README.md | 10 +++--- src/main.rs | 88 ++++++++++++++++++++++++++--------------------------- 4 files changed, 135 insertions(+), 49 deletions(-) diff --git a/Cargo.lock b/Cargo.lock index 59ca933c..fae0d81c 100644 --- a/Cargo.lock +++ b/Cargo.lock @@ -589,6 +589,17 @@ version = "0.3.31" source = "registry+https://github.com/rust-lang/crates.io-index" checksum = "9e5c1b78ca4aae1ac06c48a526a655760685149f0d465d21f37abfe57ce075c6" +[[package]] +name = "futures-macro" +version = "0.3.31" +source = "registry+https://github.com/rust-lang/crates.io-index" +checksum = "162ee34ebcb7c64a8abebc059ce0fee27c2262618d7b60ed8faf72fef13c3650" +dependencies = [ + "proc-macro2", + "quote", + "syn", +] + [[package]] name = "futures-sink" version = "0.3.31" @@ -601,6 +612,12 @@ version = "0.3.31" source = "registry+https://github.com/rust-lang/crates.io-index" checksum = "f90f7dce0722e95104fcb095585910c0977252f286e354b5e3bd38902cd99988" +[[package]] +name = "futures-timer" +version = "3.0.3" +source = "registry+https://github.com/rust-lang/crates.io-index" +checksum = "f288b0a4f20f9a56b5d1da57e2227c661b7b16168e2f72365f57b63326e29b24" + [[package]] name = "futures-util" version = "0.3.31" @@ -610,6 +627,7 @@ dependencies = [ "futures-channel", "futures-core", "futures-io", + "futures-macro", "futures-sink", "futures-task", "memchr", @@ -657,6 +675,12 @@ version = "0.31.1" source = "registry+https://github.com/rust-lang/crates.io-index" checksum = "07e28edb80900c19c28f1072f2e8aeca7fa06b23cd4169cefe1af5aa3260783f" +[[package]] +name = "glob" +version = "0.3.2" +source = "registry+https://github.com/rust-lang/crates.io-index" +checksum = "a8d1add55171497b4705a648c6b583acafb01d58050a51727785f0b2c8e0a2b2" + [[package]] name = "h2" version = "0.3.26" @@ -1283,6 +1307,15 @@ dependencies = [ "zerovec", ] +[[package]] +name = "proc-macro-crate" +version = "3.3.0" +source = "registry+https://github.com/rust-lang/crates.io-index" +checksum = "edce586971a4dfaa28950c6f18ed55e0406c1ab88bbce2c6f6293a7aaba73d35" +dependencies = [ + "toml_edit", +] + [[package]] name = "proc-macro2" version = "1.0.95" @@ -1358,6 +1391,12 @@ version = "0.8.5" source = "registry+https://github.com/rust-lang/crates.io-index" checksum = "2b15c43186be67a4fd63bee50d0303afffcef381492ebe2c5d87f324e1b8815c" +[[package]] +name = "relative-path" +version = "1.9.3" +source = "registry+https://github.com/rust-lang/crates.io-index" +checksum = "ba39f3699c378cd8970968dcbff9c43159ea4cfbd88d43c00b22f2ef10a435d2" + [[package]] name = "reqwest" version = "0.11.27" @@ -1416,12 +1455,51 @@ dependencies = [ "windows-sys 0.52.0", ] +[[package]] +name = "rstest" +version = "0.25.0" +source = "registry+https://github.com/rust-lang/crates.io-index" +checksum = "6fc39292f8613e913f7df8fa892b8944ceb47c247b78e1b1ae2f09e019be789d" +dependencies = [ + "futures-timer", + "futures-util", + "rstest_macros", + "rustc_version", +] + +[[package]] +name = "rstest_macros" +version = "0.25.0" +source = "registry+https://github.com/rust-lang/crates.io-index" +checksum = "1f168d99749d307be9de54d23fd226628d99768225ef08f6ffb52e0182a27746" +dependencies = [ + "cfg-if", + "glob", + "proc-macro-crate", + "proc-macro2", + "quote", + "regex", + "relative-path", + "rustc_version", + "syn", + "unicode-ident", +] + [[package]] name = "rustc-demangle" version = "0.1.24" source = "registry+https://github.com/rust-lang/crates.io-index" checksum = "719b953e2095829ee67db738b3bfa9fa368c94900df327b3f07fe6e794d2fe1f" +[[package]] +name = "rustc_version" +version = "0.4.1" +source = "registry+https://github.com/rust-lang/crates.io-index" +checksum = "cfcb3a22ef46e85b45de6ee7e79d063319ebb6594faafcf1c225ea92ab6e9b92" +dependencies = [ + "semver", +] + [[package]] name = "rustix" version = "1.0.7" @@ -1541,6 +1619,12 @@ dependencies = [ "libc", ] +[[package]] +name = "semver" +version = "1.0.26" +source = "registry+https://github.com/rust-lang/crates.io-index" +checksum = "56e6fa9c48d24d85fb3de5ad847117517440f6beceb7798af16b4a87d616b8d0" + [[package]] name = "serde" version = "1.0.219" @@ -2079,6 +2163,7 @@ dependencies = [ "ortho_config", "regex", "reqwest", + "rstest", "serde", "serde_json", "serial_test", diff --git a/Cargo.toml b/Cargo.toml index 41f515b0..3a2d6def 100644 --- a/Cargo.toml +++ b/Cargo.toml @@ -31,6 +31,7 @@ yaml = ["toml", "ortho_config/yaml"] [dev-dependencies] tempfile = "3.20.0" serial_test = "3.2.0" +rstest = "0.25.0" [lints.clippy] # make every pedantic lint emit a warning diff --git a/README.md b/README.md index 22a9ae53..8d4ef459 100644 --- a/README.md +++ b/README.md @@ -1,7 +1,7 @@ # View Komments (vk) `vk` stands for **View Komments** because `vc` was already taken back in the -1970s and no one argues with a greybeard. This command line tool fetches +1970s, and no one argues with a greybeard. This command line tool fetches unresolved GitHub code review comments for a pull request and displays them with colourful terminal markdown using [Termimad](https://crates.io/crates/termimad). @@ -21,9 +21,9 @@ 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 +* `pr` — show unresolved pull request comments. A summary of files and comment counts is printed first. -* `issue` - read a GitHub issue (**to do**) +* `issue` — read a GitHub issue (**to do**) If you pass just a pull request number, `vk` tries to work out which repository you meant. It first examines `.git/FETCH_HEAD` for a GitHub remote URL and, if @@ -34,7 +34,7 @@ Failing that, it falls back to the configured repository (`--repo` or neither source is available, `vk` will refuse to run with only a number. `vk` uses the GitHub GraphQL API. Set `GITHUB_TOKEN` to authenticate. If it's -not set you'll get a warning and anonymous requests may be rate limited. +not set, you'll get a warning and anonymous requests may be rate limited. The token only needs read access. Select the `public_repo` scope (or `repo` for private repositories). See [docs/GITHUB_TOKEN.md](docs/GITHUB_TOKEN.md) for a @@ -62,5 +62,5 @@ cargo install --path . ## License -This project is licensed under the ISC License. See +This project is licensed under the ISC Licence. See [LICENSE](LICENSE) for details. diff --git a/src/main.rs b/src/main.rs index dacd0fe5..1303cb75 100644 --- a/src/main.rs +++ b/src/main.rs @@ -249,14 +249,14 @@ struct Issue { body: String, } -#[derive(Deserialize)] +#[derive(Debug, Deserialize, Default)] struct ReviewThreadConnection { nodes: Vec, #[serde(rename = "pageInfo")] page_info: PageInfo, } -#[derive(Deserialize)] +#[derive(Debug, Deserialize, Default)] struct ReviewThread { id: String, #[serde(rename = "isResolved")] @@ -265,14 +265,14 @@ struct ReviewThread { comments: CommentConnection, } -#[derive(Deserialize)] +#[derive(Debug, Deserialize, Default)] struct CommentConnection { nodes: Vec, #[serde(rename = "pageInfo")] page_info: PageInfo, } -#[derive(Deserialize)] +#[derive(Debug, Deserialize, Default)] struct ReviewComment { body: String, #[serde(rename = "diffHunk")] @@ -286,7 +286,7 @@ struct ReviewComment { author: Option, } -#[derive(Deserialize)] +#[derive(Debug, Deserialize, Default)] struct PageInfo { #[serde(rename = "hasNextPage")] has_next_page: bool, @@ -294,17 +294,17 @@ struct PageInfo { end_cursor: Option, } -#[derive(Deserialize)] +#[derive(Debug, Deserialize, Default)] struct User { login: String, } -#[derive(Deserialize)] +#[derive(Debug, Deserialize, Default)] struct CommentNodeWrapper { node: Option, } -#[derive(Deserialize)] +#[derive(Debug, Deserialize, Default)] struct CommentNode { comments: CommentConnection, } @@ -782,6 +782,7 @@ fn locale_is_utf8() -> bool { #[cfg(test)] mod tests { use super::*; + use rstest::*; use std::fmt::Write; use std::fs; use tempfile::tempdir; @@ -1018,47 +1019,46 @@ mod tests { assert_eq!(number, 8); } - #[test] - fn summarize_files_counts_comments() { - fn comment(path: &str) -> ReviewComment { - ReviewComment { - body: String::new(), - diff_hunk: String::new(), - original_position: None, - position: None, - path: path.into(), - url: String::new(), - author: None, - } + #[fixture] + fn review_comment(#[default("test.rs")] path: &str) -> ReviewComment { + ReviewComment { + path: path.into(), + ..Default::default() } + } - let threads = vec![ - ReviewThread { - id: String::new(), - is_resolved: false, - comments: CommentConnection { - nodes: vec![comment("a.rs"), comment("b.rs")], - page_info: PageInfo { - has_next_page: false, - end_cursor: None, - }, - }, + #[rstest] + #[case(vec![], vec![])] + #[case( + vec![ReviewThread { + comments: CommentConnection { + nodes: vec![review_comment("a.rs"), review_comment("b.rs")], + ..Default::default() }, - ReviewThread { - id: String::new(), - is_resolved: false, - comments: CommentConnection { - nodes: vec![comment("a.rs")], - page_info: PageInfo { - has_next_page: false, - end_cursor: None, - }, - }, + ..Default::default() + }], + vec![("a.rs".into(), 1), ("b.rs".into(), 1)] + )] + #[case( + vec![ReviewThread { + comments: CommentConnection { + nodes: vec![ + review_comment("a.rs"), + review_comment("a.rs"), + review_comment("b.rs"), + ], + ..Default::default() }, - ]; - + ..Default::default() + }], + vec![("a.rs".into(), 2), ("b.rs".into(), 1)] + )] + fn summarize_files_counts_comments( + #[case] threads: Vec, + #[case] expected: Vec<(String, usize)>, + ) { let summary = summarize_files(&threads); - assert_eq!(summary, vec![("a.rs".into(), 2), ("b.rs".into(), 1)]); + assert_eq!(summary, expected); } #[test]