Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

config: support custom configuration of diff #116

Open
wants to merge 1 commit into
base: main
Choose a base branch
from

Conversation

tommilligan
Copy link
Collaborator

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 tommilligan self-assigned this May 7, 2023
@l4l
Copy link

l4l commented May 16, 2023

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

@tommilligan
Copy link
Collaborator Author

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?

@l4l
Copy link

l4l commented May 16, 2023

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.

@not-my-profile
Copy link

not-my-profile commented Aug 18, 2023

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).

@tommilligan
Copy link
Collaborator Author

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.

@not-my-profile
Copy link

not-my-profile commented Aug 18, 2023

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.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants