Merge branch 'master' into trimming

cargo-insta/Cargo.toml

@@ -30,8 +30,10 @@ tempfile = "3.5.0"
 semver = {version = "1.0.7", features = ["serde"]}
 lazy_static = "1.4.0"
 clap = { workspace=true }
+itertools = "0.10.0"
 
 [dev-dependencies]
 walkdir = "2.3.1"
 similar= "2.2.1"
 itertools = "0.10.0"
+termcolor = "1.1.2"

cargo-insta/src/cargo.rs

@@ -1,9 +1,11 @@
 use std::path::PathBuf;
 
 pub(crate) use cargo_metadata::Package;
+use itertools::Itertools;
 
 /// Find snapshot roots within a package
-// (I'm not sure how necessary this is; relative to just using all paths?)
+// We need this because paths are not always conventional — for example cargo
+// can reference artifacts that are outside of the package root.
 pub(crate) fn find_snapshot_roots(package: &Package) -> Vec<PathBuf> {
     let mut roots = Vec::new();
 
@@ -38,13 +40,19 @@ pub(crate) fn find_snapshot_roots(package: &Package) -> Vec<PathBuf> {
     // we would encounter paths twice.  If we don't skip them here we run
     // into issues where the existence of a build script causes a snapshot
     // to be picked up twice since the same path is determined.  (GH-15)
-    roots.sort_by_key(|x| x.as_os_str().len());
-    let mut reduced_roots = vec![];
-    for root in roots {
-        if !reduced_roots.iter().any(|x| root.starts_with(x)) {
-            reduced_roots.push(root);
-        }
-    }
-
-    reduced_roots
+    let canonical_roots: Vec<_> = roots
+        .iter()
+        .filter_map(|x| x.canonicalize().ok())
+        .sorted_by_key(|x| x.as_os_str().len())
+        .collect();
+
+    canonical_roots
+        .clone()
+        .into_iter()
+        .filter(|root| {
+            !canonical_roots
+                .iter()
+                .any(|x| root.starts_with(x) && root != x)
+        })
+        .collect()
 }

cargo-insta/src/cli.rs

@@ -83,7 +83,7 @@ enum Command {
 
 #[derive(Args, Debug, Clone)]
 struct TargetArgs {
-    /// Path to Cargo.toml
+    /// Path to `Cargo.toml`
     #[arg(long, value_name = "PATH")]
     manifest_path: Option<PathBuf>,
     /// Explicit path to the workspace root
@@ -95,7 +95,7 @@ struct TargetArgs {
     /// Work on all packages in the workspace
     #[arg(long)]
     workspace: bool,
-    /// Alias for --workspace (deprecated)
+    /// Alias for `--workspace` (deprecated)
     #[arg(long)]
     all: bool,
     /// Also walk into ignored paths.

cargo-insta/src/container.rs

@@ -47,7 +47,9 @@ impl PendingSnapshot {
 /// single rust file.
 #[derive(Debug)]
 pub(crate) struct SnapshotContainer {
-    snapshot_path: PathBuf,
+    // Path of the pending snapshot file (generally a `.new` or `.pending-snap` file)
+    pending_path: PathBuf,
+    // Path of the target snapshot file (generally a `.snap` file)
     target_path: PathBuf,
     kind: SnapshotKind,
     snapshots: Vec<PendingSnapshot>,
@@ -56,7 +58,7 @@ pub(crate) struct SnapshotContainer {
 
 impl SnapshotContainer {
     pub(crate) fn load(
-        snapshot_path: PathBuf,
+        pending_path: PathBuf,
         target_path: PathBuf,
         kind: SnapshotKind,
     ) -> Result<SnapshotContainer, Box<dyn Error>> {
@@ -68,7 +70,7 @@ impl SnapshotContainer {
                 } else {
                     Some(Snapshot::from_file(&target_path)?)
                 };
-                let new = Snapshot::from_file(&snapshot_path)?;
+                let new = Snapshot::from_file(&pending_path)?;
                 snapshots.push(PendingSnapshot {
                     id: 0,
                     old,
@@ -79,7 +81,7 @@ impl SnapshotContainer {
                 None
             }
             SnapshotKind::Inline => {
-                let mut pending_vec = PendingInlineSnapshot::load_batch(&snapshot_path)?;
+                let mut pending_vec = PendingInlineSnapshot::load_batch(&pending_path)?;
                 let mut have_new = false;
 
                 let rv = if fs::metadata(&target_path).is_ok() {
@@ -111,16 +113,16 @@ impl SnapshotContainer {
                 // The runtime code will issue something like this:
                 //   PendingInlineSnapshot::new(None, None, line).save(pending_snapshots)?;
                 if !have_new {
-                    fs::remove_file(&snapshot_path)
-                        .map_err(|e| ContentError::FileIo(e, snapshot_path.to_path_buf()))?;
+                    fs::remove_file(&pending_path)
+                        .map_err(|e| ContentError::FileIo(e, pending_path.to_path_buf()))?;
                 }
 
                 rv
             }
         };
 
         Ok(SnapshotContainer {
-            snapshot_path,
+            pending_path,
             target_path,
             kind,
             snapshots,
@@ -141,7 +143,7 @@ impl SnapshotContainer {
 
     pub(crate) fn snapshot_sort_key(&self) -> impl Ord + '_ {
         let path = self
-            .snapshot_path
+            .pending_path
             .file_name()
             .and_then(|x| x.to_str())
             .unwrap_or_default();
@@ -201,32 +203,32 @@ impl SnapshotContainer {
                 patcher.save()?;
             }
             if did_skip {
-                PendingInlineSnapshot::save_batch(&self.snapshot_path, &new_pending)?;
+                PendingInlineSnapshot::save_batch(&self.pending_path, &new_pending)?;
             } else {
-                try_removing_snapshot(&self.snapshot_path);
+                try_removing_snapshot(&self.pending_path);
             }
         } else {
             // should only be one or this is weird
             debug_assert!(self.snapshots.len() == 1);
             for snapshot in self.snapshots.iter() {
                 match snapshot.op {
                     Operation::Accept => {
-                        let snapshot = Snapshot::from_file(&self.snapshot_path).map_err(|e| {
+                        let snapshot = Snapshot::from_file(&self.pending_path).map_err(|e| {
                             // If it's an IO error, pass a ContentError back so
                             // we get a slightly clearer error message
                             match e.downcast::<std::io::Error>() {
                                 Ok(io_error) => Box::new(ContentError::FileIo(
                                     *io_error,
-                                    self.snapshot_path.to_path_buf(),
+                                    self.pending_path.to_path_buf(),
                                 )),
                                 Err(other_error) => other_error,
                             }
                         })?;
                         snapshot.save(&self.target_path)?;
-                        try_removing_snapshot(&self.snapshot_path);
+                        try_removing_snapshot(&self.pending_path);
                     }
                     Operation::Reject => {
-                        try_removing_snapshot(&self.snapshot_path);
+                        try_removing_snapshot(&self.pending_path);
                     }
                     Operation::Skip => {}
                 }

cargo-insta/src/inline.rs

@@ -149,19 +149,22 @@ impl FilePatcher {
         impl Visitor {
             fn scan_nested_macros(&mut self, tokens: &[TokenTree]) {
                 for idx in 0..tokens.len() {
+                    // Look for the start of a macro (potential snapshot location)
                     if let Some(TokenTree::Ident(_)) = tokens.get(idx) {
                         if let Some(TokenTree::Punct(ref punct)) = tokens.get(idx + 1) {
                             if punct.as_char() == '!' {
                                 if let Some(TokenTree::Group(ref group)) = tokens.get(idx + 2) {
+                                    // Found a macro, determine its indentation
                                     let indentation = scan_for_path_start(tokens, idx);
+                                    // Extract tokens from the macro arguments
                                     let tokens: Vec<_> = group.stream().into_iter().collect();
+                                    // Try to extract a snapshot, passing the calculated indentation
                                     self.try_extract_snapshot(&tokens, indentation);
                                 }
                             }
                         }
                     }
                 }
-
                 for token in tokens {
                     // recurse into groups
                     if let TokenTree::Group(group) = token {

cargo-insta/src/main.rs

@@ -1,3 +1,6 @@
+#![warn(clippy::doc_markdown)]
+#![warn(rustdoc::all)]
+
 //! <div align="center">
 //!  <img src="https://github.com/mitsuhiko/insta/blob/master/assets/logo.png?raw=true" width="250" height="250">
 //!  <p><strong>cargo-insta: review tool for insta, a snapshot testing library for Rust</strong></p>

cargo-insta/src/utils.rs

@@ -48,7 +48,7 @@ lazy_static! {
     pub static ref INSTA_VERSION: Version = read_insta_version().unwrap();
 }
 
-/// cargo-insta version
+/// `cargo-insta` version
 // We could put this in a lazy_static
 pub(crate) fn cargo_insta_version() -> String {
     env!("CARGO_PKG_VERSION").to_string()

cargo-insta/src/walk.rs

@@ -28,23 +28,25 @@ pub(crate) fn find_pending_snapshots<'a>(
     flags: FindFlags,
 ) -> impl Iterator<Item = Result<SnapshotContainer, Box<dyn Error>>> + 'a {
     make_snapshot_walker(package_root, extensions, flags)
-        .filter_map(|e| e.ok())
-        .filter_map(move |e| {
-            let fname = e.file_name().to_string_lossy();
-            if fname.ends_with(".new") {
-                let new_path = e.into_path();
-                let old_path = new_path.clone().with_extension("");
+        .filter_map(Result::ok)
+        .filter_map(|entry| {
+            let fname = entry.file_name().to_string_lossy();
+            let path = entry.clone().into_path();
+
+            #[allow(clippy::manual_map)]
+            if let Some(new_fname) = fname.strip_suffix(".new") {
                 Some(SnapshotContainer::load(
-                    new_path,
-                    old_path,
+                    path.clone(),
+                    path.with_file_name(new_fname),
                     SnapshotKind::File,
                 ))
-            } else if fname.starts_with('.') && fname.ends_with(".pending-snap") {
-                let mut target_path = e.path().to_path_buf();
-                target_path.set_file_name(&fname[1..fname.len() - 13]);
+            } else if let Some(new_fname) = fname
+                .strip_prefix('.')
+                .and_then(|f| f.strip_suffix(".pending-snap"))
+            {
                 Some(SnapshotContainer::load(
-                    e.path().to_path_buf(),
-                    target_path,
+                    path.clone(),
+                    path.with_file_name(new_fname),
                     SnapshotKind::Inline,
                 ))
             } else {