Use the 'pretty-assertions' crate

[chshersh]

This package provides a better UX for failing tests.

• https://github.com/rust-pretty-assertions/rust-pretty-assertions

Honestly, I think it should be the default. But it's possible to use this crate only for tests and not for users so it shouldn't affect people install tool-sync with cargo install.

[MitchellBerend]

I was thinking of refactoring the way errors are printed to stdout/err if that is not included in #97, we can include this in the same refactor and I think we can also just add this as a dev dependency so it will only be included in the binary that gets built with cargo test

[chshersh]

97 looks pretty separate to me 👍🏻

This issue can be implemented separately. I imagine a pretty small diff. It's just adding proper imports, dev dependencies and quickly verifying that it does work as expected.

Come to think of it, it would be nice to enforce the usage of pretty-assertions everywhere in the codebase so it would be impossible to use plain assert_eq! 🤔 Is there a way to do this in Rust? Kinda like a custom listing rule.

[MitchellBerend]

Come to think of it, it would be nice to enforce the usage of pretty-assertions everywhere in the codebase so it would be impossible to use plain assert_eq! thinking Is there a way to do this in Rust?

I don't know of any but this can be caught in review too.

[chshersh]

Looks like we can use clippy configuration options disallow_methods to forbid the usage of specific functions:

• https://rust-lang.github.io/rust-clippy/master/#disallowed_method

My plan would be the following:

• Try this config option to see if it can forbid assert_eq!
• Try with pretty-assertions to make sure that it doesn't warn on assert_eq! from an external library

If this works — great! If not, we need to regroup and think 🤔

I'm not feeling confident in catching such kinds of problems during the review. There's too much I can remember to think about while reviewing the code 😞

[MitchellBerend]

Unless Im doing something wrong I can't seem to get clippy to run on tests. Note that the top of the test section in src/config/toml.rs contains the following lint option:

#[cfg(test)]
mod tests {
    #![deny(clippy::disallowed_methods)]
    use super::*;

$ cat .clippy.toml
disallowed-methods = [
    "std::assert_eq",
    "std::assert_ne",
    "std::assert_str_eq",

    "std::boxed::Box::new",
]

#![deny(clippy::disallowed_methods)]

use std::collections::BTreeMap;
use std::fmt::{Display, Formatter};
use std::fs;
use std::path::PathBuf;
use toml::{map::Map, Value};

use crate::config::schema::{Config, ConfigAsset};
use crate::infra::err;
use crate::model::asset_name::AssetName;

#[derive(Debug, PartialEq, Eq)]
pub enum TomlError {
    IO(String),
    Parse(toml::de::Error),
    Decode,
}

impl Display for TomlError {
    fn fmt(&self, f: &mut Formatter<'_>) -> std::fmt::Result {
        let _: std::boxed::Box<String> = std::boxed::Box::new(String::new());

        match self {
            TomlError::IO(e) => write!(f, "[IO Error] {}", e),
            TomlError::Parse(e) => write!(f, "[Parsing Error] {}", e),
            TomlError::Decode => write!(f, "[Decode Error]"),
        }
    }
}

$ cargo clippy
    Checking tool-sync v0.2.0 (/home/mitchell/rust/tool-sync)
error: use of a disallowed method `std::boxed::Box::new`
  --> src/config/toml.rs:23:42
   |
23 |         let _: std::boxed::Box<String> = std::boxed::Box::new(String::new());
   |                                          ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
   |
note: the lint level is defined here
  --> src/config/toml.rs:1:9
   |
1  | #![deny(clippy::disallowed_methods)]
   |         ^^^^^^^^^^^^^^^^^^^^^^^^^^
   = help: for further information visit https://rust-lang.github.io/rust-clippy/master/index.html#disallowed_methods

warning: use of a disallowed method `std::boxed::Box::new`
  --> src/lib.rs:24:26
   |
24 |     let _: Box<String> = Box::new(String::new());
   |                          ^^^^^^^^^^^^^^^^^^^^^^^
   |
   = note: `#[warn(clippy::disallowed_methods)]` on by default
   = help: for further information visit https://rust-lang.github.io/rust-clippy/master/index.html#disallowed_methods

warning: `tool-sync` (lib) generated 1 warning
error: could not compile `tool-sync` due to previous error; 1 warning emitted

$ cargo clippy --tests
    Checking tool-sync v0.2.0 (/home/mitchell/rust/tool-sync)
error: use of a disallowed method `std::boxed::Box::new`
  --> src/config/toml.rs:23:42
   |
23 |         let _: std::boxed::Box<String> = std::boxed::Box::new(String::new());
   |                                          ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
   |
note: the lint level is defined here
  --> src/config/toml.rs:1:9
   |
1  | #![deny(clippy::disallowed_methods)]
   |         ^^^^^^^^^^^^^^^^^^^^^^^^^^
   = help: for further information visit https://rust-lang.github.io/rust-clippy/master/index.html#disallowed_methods

warning: `assert!(false, ..)` should probably be replaced
   --> src/config/toml.rs:178:17
    |
178 |                 assert!(false, "Unexpected succces")
    |                 ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
    |
    = note: `#[warn(clippy::assertions_on_constants)]` on by default
    = help: use `panic!(..)` or `unreachable!(..)`
    = help: for further information visit https://rust-lang.github.io/rust-clippy/master/index.html#assertions_on_constants

warning: `assert!(true)` will be optimized out by the compiler
   --> src/config/toml.rs:181:17
    |
181 |                 assert!(true, "Exepected a parsing error")
    |                 ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
    |
    = help: remove it
    = help: for further information visit https://rust-lang.github.io/rust-clippy/master/index.html#assertions_on_constants

warning: use of a disallowed method `std::boxed::Box::new`
  --> src/lib.rs:24:26
   |
24 |     let _: Box<String> = Box::new(String::new());
   |                          ^^^^^^^^^^^^^^^^^^^^^^^
   |
   = note: `#[warn(clippy::disallowed_methods)]` on by default
   = help: for further information visit https://rust-lang.github.io/rust-clippy/master/index.html#disallowed_methods

warning: `tool-sync` (lib) generated 1 warning
error: could not compile `tool-sync` due to previous error; 1 warning emitted
warning: build failed, waiting for other jobs to finish...
warning: `tool-sync` (lib test) generated 3 warnings (1 duplicate)
error: could not compile `tool-sync` due to previous error; 3 warnings emitted

[chshersh]

@MitchellBerend I found this clippy issue:

• https://github.com/rust-lang/rust-clippy/issues/8065

It suggests that there's a hidden option --tests that you can use to run clippy in tests (and it doesn't run in tests by default). Could you try with it?

[MitchellBerend]

I did, its the last output in the previous comment.

[chshersh]

Oh, didn't notice 😮 In that case, I'm not sure how to fix it 🤔