rust-pretty-assertions icon indicating copy to clipboard operation
rust-pretty-assertions copied to clipboard

config: support custom configuration of diff

Open tommilligan opened this issue 2 years ago • 6 comments

Supersedes #79

Relates to #38, #105, #114

This PR proposes an extensible system for customising diff output, by prefixing invocation with a config = ... argument.

use pretty_assertions::assert_eq;
use pretty_assertions::config::{Config, LineSymbol};

// Uses '+' and '-' symbols, instead of '<' and '>'
let config = Config::default().line_symbol(LineSymbol::Sign);
assert_eq!(config = config, 42, 43);
thread 'main' panicked at 'assertion failed: `(left == right)`

Diff - left / right + :
-42
+43

It's inspired by the tracing macros, which deal with the log::log! macros not being extensible with further suffix arguments, by adding prefix keyword arguments instead.

This is the best design I've come across for extending standard library macros that take an arbitrary number of positional arguments - the Config struct itself can be extended in future by adding new builder methods.

tommilligan avatar May 07 '23 16:05 tommilligan

Would be really cool if the config could also be set via environments so no need to recompile.

l4l avatar May 16 '23 10:05 l4l

Would be really cool if the config could also be set via environments so no need to recompile.

Good point, I can see how having something similar to the RUST_LOG environment variable would be useful.

I can think of two kinds of API here:

  • Multiple keys, one per setting: RUST_ASSERT_LINE_SYMBOL=sign, RUST_ASSERT_CONTEXT=minimal
  • One key, internal config format (sort of TOML-ish): RUST_ASSERT=line_symbol=sign,context=minimal

I think the second option where we have one key that we parse if nicer. I wouldn't want to pull in an external crate for this, but simple key-value parsing is trivial.

Would that work for your use case?

tommilligan avatar May 16 '23 10:05 tommilligan

Yeah, that's a good one. Also I would expect the following behavior:

  • envs are used even if no config is passed explicitly;
  • then if certain env value is not set, the default value is used;
  • envs values are overwriting settings.

l4l avatar May 16 '23 11:05 l4l

I think the suggested syntax is very problematic:

assert_eq!(config = config, 42, 43);

When reading this code I'd assume that it asserts that config equals config and be very confused what 42 and 43 have to do with that.

To be honest I really don't see the point of overloading the usage of the macros that mimic the std macros. I think it would make much more sense to provide an alternative non-macro based API, like snapbox does with snapbox::Assert. However if you actually want so much fine-grained control over the diff I think it might actually make sense that you do the panicking yourself (e.g. similar_asserts takes this approach with similar_asserts::SimpleDiff).

not-my-profile avatar Aug 18 '23 04:08 not-my-profile

To be honest I really don't see the point of overloading the usage of the macros that mimic the std macros.

That's very valid. I do like how having a drop-in replacement eases transition burden from the standard macros, but you're right that there's no reason to make that the only API.

Some sort of builder style API seems reasonable in that case: we already have a non-macro interface that's not publicly exposed.

tommilligan avatar Aug 18 '23 06:08 tommilligan

Right ... though it's not just the easy transition from std to pretty_assertions that's nice. I also appreciate how you can switch back and forth between std, pretty_assertions and even other crates like similar-asserts by just changing the imports. When it comes to code I think clarity is one of the most important goals, and from that standpoint I think that any overloading of the std compatible macros is undesirable since it breaks expectations.

not-my-profile avatar Aug 18 '23 07:08 not-my-profile