fix: propagate errors (#47)

This follows idiomatic rust [error handling] by using the [anyhow library].

[error handling]: https://doc.rust-lang.org/book/ch09-02-recoverable-errors-with-result.html
[anyhow library]: https://crates.io/crates/anyhow

* setup non-system python in CI's sdist build job
* improve log output for silent errors
* remove trailing '/' in endpoint url
* add user-agent header; adjust auth haeader value
* skip serializing `ReviewComment::start_line` if it is null
* add tests; review error handling
* fix review dismissal and thread comment updating
* use test profiles
* upload coverage reports from windows runners to codecov.io

Author: 2bndy5

.readthedocs.yaml renamed to .config/.readthedocs.yaml

@@ -0,0 +1,31 @@
+# required minimum nextest version
+nextest-version = "0.9.77"
+
+[profile.default]
+# A profile to run most tests, except tests that run longer than 10 seconds
+default-filter = "not test(#*rate_limit_secondary) - test(#git::test::with_*)"
+
+# This will flag any test that runs longer than 10 seconds. Useful when writing new tests.
+slow-timeout = "10s"
+
+[profile.ci]
+# A profile to run only tests that use clang-tidy and/or clang-format
+# NOTE: This profile is intended to keep CI runtime low. Locally, use default or all profiles
+
+# This is all tests in tests/ folder + unit test for --extra-args.
+default-filter = "kind(test) + test(#*use_extra_args)"
+
+# show wich tests were skipped
+status-level = "skip"
+
+# show log output from each test
+success-output = "final"
+failure-output = "immediate-final"
+
+[profile.all]
+# A profile to run all tests (including tests that run longer than 10 seconds)
+default-filter = "all()"
+
+# Revert slow-timeout to nextest default value.
+# Otherwise, default profile value (10s) is inherited.
+slow-timeout = "60s"

cliff.toml renamed to .config/cliff.toml

@@ -54,7 +54,7 @@ jobs:
         uses: orhun/git-cliff-action@v4
         id: git-cliff
         with:
-          config: cliff.toml
+          config: .config/cliff.toml
           args: --unreleased
         env:
           OUTPUT: ${{ runner.temp }}/changes.md

.config/nextest.toml

@@ -120,7 +120,16 @@ def main():
     subprocess.run(["napi", "version"], cwd="node-binding", check=True)
     print("Updated version in node-binding/**package.json")
 
-    subprocess.run(["git", "cliff", "--tag", Updater.new_version], check=True)
+    subprocess.run(
+        [
+            "git-cliff",
+            "--config",
+            ".config/cliff.toml",
+            "--tag",
+            Updater.new_version,
+        ],
+        check=True,
+    )
     print("Updated CHANGELOG.md")
 
     subprocess.run(["git", "add", "--all"], check=True)

.github/workflows/bump-n-release.yml

@@ -155,6 +155,9 @@ jobs:
     runs-on: ubuntu-latest
     steps:
       - uses: actions/checkout@v4
+      - uses: actions/setup-python@v5
+        with:
+          python-version: 3.x
       - name: Build sdist
         uses: PyO3/maturin-action@v1
         with:

.github/workflows/bump_version.py

@@ -88,7 +88,7 @@ jobs:
         if: runner.os == 'Linux'
         env:
           CLANG_VERSION: '7'
-        run: just test
+        run: just test ci
 
       - name: Install clang v8
         if: runner.os == 'Linux'
@@ -100,7 +100,7 @@ jobs:
         if: runner.os == 'Linux'
         env:
           CLANG_VERSION: '8'
-        run: just test
+        run: just test ci
 
       - name: Install clang v9
         uses: cpp-linter/cpp_linter_rs/install-clang-action@main
@@ -110,7 +110,7 @@ jobs:
       - name: Collect Coverage for clang v9
         env:
           CLANG_VERSION: '9'
-        run: just test
+        run: just test ci
 
       - name: Install clang v10
         uses: cpp-linter/cpp_linter_rs/install-clang-action@main
@@ -120,7 +120,7 @@ jobs:
       - name: Collect Coverage for clang v10
         env:
           CLANG_VERSION: '10'
-        run: just test
+        run: just test ci
 
       - name: Install clang 11
         uses: cpp-linter/cpp_linter_rs/install-clang-action@main
@@ -130,7 +130,7 @@ jobs:
       - name: Collect Coverage for clang v11
         env:
           CLANG_VERSION: '11'
-        run: just test
+        run: just test ci
 
       - name: Install clang 12
         uses: cpp-linter/cpp_linter_rs/install-clang-action@main
@@ -140,7 +140,7 @@ jobs:
       - name: Collect Coverage for clang v12
         env:
           CLANG_VERSION: '12'
-        run: just test
+        run: just test ci
 
       - name: Install clang 13
         uses: cpp-linter/cpp_linter_rs/install-clang-action@main
@@ -150,7 +150,7 @@ jobs:
       - name: Collect Coverage for clang v13
         env:
           CLANG_VERSION: '13'
-        run: just test
+        run: just test ci
 
       - name: Install clang 14
         uses: cpp-linter/cpp_linter_rs/install-clang-action@main
@@ -160,7 +160,7 @@ jobs:
       - name: Collect Coverage for clang v14
         env:
           CLANG_VERSION: '14'
-        run: just test
+        run: just test ci
 
       - name: Install clang 15
         uses: cpp-linter/cpp_linter_rs/install-clang-action@main
@@ -170,7 +170,7 @@ jobs:
       - name: Collect Coverage for clang v15
         env:
           CLANG_VERSION: '15'
-        run: just test
+        run: just test ci
 
       - name: Install clang 16
         uses: cpp-linter/cpp_linter_rs/install-clang-action@main
@@ -180,7 +180,7 @@ jobs:
       - name: Collect Coverage for clang v16
         env:
           CLANG_VERSION: '16'
-        run: just test
+        run: just test ci
 
       - name: Install clang 17
         uses: cpp-linter/cpp_linter_rs/install-clang-action@main
@@ -190,7 +190,7 @@ jobs:
       - name: Collect Coverage for clang v17
         env:
           CLANG_VERSION: '17'
-        run: just test
+        run: just test ci
 
       - name: Install clang 18
         uses: cpp-linter/cpp_linter_rs/install-clang-action@main
@@ -200,7 +200,7 @@ jobs:
       - name: Collect Coverage for clang v18
         env:
           CLANG_VERSION: '18'
-        run: just test --run-ignored=all
+        run: just test all
 
       - name: Generate Coverage HTML report
         run: just pretty-cov
@@ -212,13 +212,11 @@ jobs:
           path: target/llvm-cov-pretty
 
       - name: Generate Coverage lcov report
-        if: runner.os == 'Linux'
         run: |
           rm coverage.json
           just lcov
 
       - uses: codecov/codecov-action@v4
-        if: runner.os == 'Linux'
         with:
           token: ${{secrets.CODECOV_TOKEN}}
           files: lcov.info

.github/workflows/python-packaging.yml

@@ -14,6 +14,7 @@ license.workspace = true
 # See more keys and their definitions at https://doc.rust-lang.org/cargo/reference/manifest.html
 
 [dependencies]
+anyhow = "1.0.89"
 chrono = "0.4.38"
 clap = "4.5.17"
 fast-glob = "0.4.0"

.github/workflows/run-dev-tests.yml

@@ -6,6 +6,7 @@ use std::{
     sync::{Arc, Mutex, MutexGuard},
 };
 
+use anyhow::{Context, Result};
 use log::Level;
 // non-std crates
 use serde::Deserialize;
@@ -56,13 +57,15 @@ pub struct Replacement {
     ///
     /// This value is not provided by the XML output, but we calculate it after
     /// deserialization.
-    pub line: Option<usize>,
+    #[serde(default)]
+    pub line: usize,
 
     /// The column number on the line described by the [`Replacement::offset`].
     ///
     /// This value is not provided by the XML output, but we calculate it after
     /// deserialization.
-    pub cols: Option<usize>,
+    #[serde(default)]
+    pub cols: usize,
 }
 
 impl Clone for Replacement {
@@ -109,15 +112,16 @@ pub fn tally_format_advice(files: &[Arc<Mutex<FileObj>>]) -> u64 {
 pub fn run_clang_format(
     file: &mut MutexGuard<FileObj>,
     clang_params: &ClangParams,
-) -> Vec<(log::Level, String)> {
+) -> Result<Vec<(log::Level, String)>> {
     let mut cmd = Command::new(clang_params.clang_format_command.as_ref().unwrap());
     let mut logs = vec![];
     cmd.args(["--style", &clang_params.style]);
     let ranges = file.get_ranges(&clang_params.lines_changed_only);
     for range in &ranges {
         cmd.arg(format!("--lines={}:{}", range.start(), range.end()));
     }
-    cmd.arg(file.name.to_string_lossy().as_ref());
+    let file_name = file.name.to_string_lossy().to_string();
+    cmd.arg(file.name.to_path_buf().as_os_str());
     let mut patched = None;
     if clang_params.format_review {
         logs.push((
@@ -129,7 +133,7 @@ pub fn run_clang_format(
                     .as_ref()
                     .unwrap()
                     .to_str()
-                    .unwrap(),
+                    .unwrap_or_default(),
                 cmd.get_args()
                     .map(|a| a.to_str().unwrap())
                     .collect::<Vec<&str>>()
@@ -138,7 +142,7 @@ pub fn run_clang_format(
         ));
         patched = Some(
             cmd.output()
-                .expect("Failed to get fixes from clang-format")
+                .with_context(|| format!("Failed to get fixes from clang-format: {file_name}"))?
                 .stdout,
         );
     }
@@ -147,33 +151,30 @@ pub fn run_clang_format(
         log::Level::Info,
         format!(
             "Running \"{} {}\"",
-            clang_params
-                .clang_format_command
-                .as_ref()
-                .unwrap()
-                .to_str()
-                .unwrap(),
+            cmd.get_program().to_string_lossy(),
             cmd.get_args()
                 .map(|x| x.to_str().unwrap())
                 .collect::<Vec<&str>>()
                 .join(" ")
         ),
     ));
-    let output = cmd.output().unwrap();
+    let output = cmd
+        .output()
+        .with_context(|| format!("Failed to get replacements from clang-format: {file_name}"))?;
     if !output.stderr.is_empty() || !output.status.success() {
         logs.push((
             log::Level::Debug,
             format!(
                 "clang-format raised the follow errors:\n{}",
-                String::from_utf8(output.stderr).unwrap()
+                String::from_utf8_lossy(&output.stderr)
             ),
         ));
     }
     if output.stdout.is_empty() {
-        return logs;
+        return Ok(logs);
     }
     let xml = String::from_utf8(output.stdout)
-        .unwrap()
+        .with_context(|| format!("stdout from clang-format was not UTF-8 encoded: {file_name}"))?
         .lines()
         .collect::<Vec<&str>>()
         .join("");
@@ -189,11 +190,12 @@ pub fn run_clang_format(
         });
     format_advice.patched = patched;
     if !format_advice.replacements.is_empty() {
+        // get line and column numbers from format_advice.offset
         let mut filtered_replacements = Vec::new();
         for replacement in &mut format_advice.replacements {
             let (line_number, columns) = get_line_cols_from_offset(&file.name, replacement.offset);
-            replacement.line = Some(line_number);
-            replacement.cols = Some(columns);
+            replacement.line = line_number;
+            replacement.cols = columns;
             for range in &ranges {
                 if range.contains(&line_number.try_into().unwrap_or(0)) {
                     filtered_replacements.push(replacement.clone());
@@ -208,7 +210,7 @@ pub fn run_clang_format(
         format_advice.replacements = filtered_replacements;
     }
     file.format_advice = Some(format_advice);
-    logs
+    Ok(logs)
 }
 
 #[cfg(test)]
@@ -234,29 +236,29 @@ mod tests {
                     offset: 113,
                     length: 5,
                     value: Some(String::from("\n      ")),
-                    line: None,
-                    cols: None,
+                    line: 0,
+                    cols: 0,
                 },
                 Replacement {
                     offset: 147,
                     length: 0,
                     value: Some(String::from(" ")),
-                    line: None,
-                    cols: None,
+                    line: 0,
+                    cols: 0,
                 },
                 Replacement {
                     offset: 161,
                     length: 0,
                     value: None,
-                    line: None,
-                    cols: None,
+                    line: 0,
+                    cols: 0,
                 },
                 Replacement {
                     offset: 165,
                     length: 19,
                     value: Some(String::from("\n\n")),
-                    line: None,
-                    cols: None,
+                    line: 0,
+                    cols: 0,
                 },
             ],
             patched: None,