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

Print warning when we cannot print good diff #104

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

Conversation

lemolatoon
Copy link

Ref #100

If left and right are not equal, but we cannot print any diff, then make sure to print warning instead.

example:

use pretty_assertions::assert_eq;

fn main() {
    assert_eq!("\n", "\r\n");
}
thread 'main' panicked at 'assertion failed: `(left == right)`

Diff < left / right > :
Warning: values were different, but no line diff was generated. Do your values have the same line endings?

', pretty_assertions/examples/pretty_assertion.rs:4:5
note: run with `RUST_BACKTRACE=1` environment variable to display a backtrace

@colin-kiegel
Copy link
Collaborator

@lemolatoon Oh, that's quite tricky - even if a diff is shown, these differences could be hidden in the output. :-/

No Diff Case

The warning is good, but let's try to offer more - why not at least fall back to the default presentation AFTER the warning?
IMO we don't need to ask the user whether there are different line endings, if we can also tell him.

General Case

I wonder if we should try to detect "invisible" differences and implement more general fallbacks?

@lemolatoon
Copy link
Author

@colin-kiegel Thank you for replying. Let me comfirm what you suggested.

No Diff Case

So you mean Asking (Do your values have the same line endings?) is unnecessary. And Warning should be shown before.
In which case, example would be...

thread 'main' panicked at 'assertion failed: `(left == right)`

Warning: values were different, but no line diff was generated.
Diff < left / right > :

', pretty_assertions/examples/pretty_assertion.rs:4:5
note: run with `RUST_BACKTRACE=1` environment variable to display a backtrace

General Case

In another case, such as...

use pretty_assertions::assert_eq;

fn main() {
    assert_eq!("a\n", "ab\r\n");
}

As you pointed out, on current implementation, we cannot detect and print warning on this type.

What I am going to do

I am going to fix these points.

@colin-kiegel
Copy link
Collaborator

colin-kiegel commented Sep 20, 2022

@lemolatoon That's only part of what I meant - first let's keep the second part, but not as a question:

  • Warning: values were different, but no line diff was generated. It seems like these values only differ in line endings - falling back to debug output:

Then we could fallback to showing the debug-string of left and right, like the original "not pretty" assert_eq!() output´.

Warning: values were different, but no line diff was generated. It seems like these values only differ in line endings - falling back to debug output:
Diff < left / right > :
  left: `"Foo\n"`,
 right: `"Foo\r\n"`'

@lemolatoon
Copy link
Author

Now by the commit I just did. It should be ok.
example:

use pretty_assertions::assert_eq;

fn main() {
    assert_eq!("Foo\n", "Foo\r\n")
}
thread 'main' panicked at 'assertion failed: `(left == right)`

Warning: values were different, but no line diff was generated. It seems like these values only differ in line endings - falling back to debug output:
Diff < left / right > :
 left: `"Foo\n"`,
right: `"Foo\r\n"`

', pretty_assertions/examples/pretty_assertion.rs:4:5
note: run with `RUST_BACKTRACE=1` environment variable to display a backtrace

@mitsuhiko
Copy link

You might want to steal some code from similar-asserts which renders sigils for newlines if they matter: https://github.com/mitsuhiko/similar-asserts/blob/63eef1b71d2d21648c3b8c992c616a7c4b74d747/src/lib.rs#L214

@colin-kiegel
Copy link
Collaborator

@mitsuhiko Nice, thank you!

You might want to steal some code from similar-asserts which renders sigils for newlines if they matter: https://github.com/mitsuhiko/similar-asserts/blob/63eef1b71d2d21648c3b8c992c616a7c4b74d747/src/lib.rs#L214

I also like the way you both print the original left / right output AND the difference. It seems worthwile to also copy this behaviour - so far rust-pretty-assertions only prints the difference...

grafik

Off-Topic: Do you see any other areas where either rust-pretty-assertions or similar-asserts perform better and could learn from each other?


PS: @tommilligan What's your thought on all of this?

@mitsuhiko
Copy link

mitsuhiko commented Sep 29, 2022

@colin-kiegel similar-asserts pretty much only exists because of the performance issues i with with this crate in the past on large diffs and I already had similar in my dependency tree for insta. But since pretty-assertions is used in a lot of places I am exposed to it :)

I personally believe that at this point similar-asserts is the better crate, but it has much lower adoption and in the most ideal of cases they would just become the same thing.

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

4 participants