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

Add assert_str_eq macro for comparing "raw" strings #88

Merged
merged 1 commit into from
Feb 1, 2022

Conversation

x3ro
Copy link
Contributor

@x3ro x3ro commented Jan 31, 2022

As discussed in #24, pretty_assertion's assert_eq will escape line
breaks in multi-line strings, making diffs for them much harder to
read.

This commit introduces another variant, assert_str_eq, and the
associated StrComparison, which compares the raw strings, where
the newlines are not escaped.

Fixes #24

@tommilligan
Copy link
Collaborator

Hi @x3ro - thanks for your contribution, and your well thought out discussion in #24 as well.

I actually really like your proposed solution. I think previously I've felt that proposed solutions (both others and my own) have been too generic/complex and would need to be split out to separate crates. This feels actually like you're simplifying the functionality - we're literally skipping a step (where we turn whatever type into &str before we diff it), and it totally feels natural to be able to do that.

I don't have any inline feedback on your implementation as is, it looks like it does the job for &str types. My general feedback would be:

  • Docstrings need filling out still
  • We can make this solution general for all types that implement AsRef<str>. This will allow users with custom string types to still use the diffing if they wish

I've pushed a commit that makes suggestions for both these points. Please take a look and let me know if you want to discuss anything further - otherwise I'm happy to get this merged in and released.

@x3ro
Copy link
Contributor Author

x3ro commented Feb 1, 2022

Hey @tommilligan :) Thanks for the feedback and great suggestions. I had also played around with AsRef<str> during the implementation, but failed because I tried to use it in the constructor, where I then ran into issues with lifetime. I really like your solution.

The only potential issue I see is that one can instantiate StrComparison with arbitrary types and the compilation will only fail once trying to Display it, e.g.

let c = StrComparison::new(&6, &6); # works fine

# breaks
println!("{}", c);
               ^ the trait `AsRef<str>` is not implemented for `{integer}`

It's hard for me to judge whether this would actually be a problem, since this would only impact people who are using StrComparison directly, without the macros. In order to avoid this, we could apply the trait bound for StrComparison more generally:

pub struct StrComparison<'a, TLeft, TRight>
where
    TLeft: AsRef<str> + ?Sized,
    TRight: AsRef<str> + ?Sized,
{
    left: &'a TLeft,
    right: &'a TRight,
}

impl<'a, TLeft, TRight> StrComparison<'a, TLeft, TRight>
where
    TLeft: AsRef<str> + ?Sized,
    TRight: AsRef<str> + ?Sized,
{
    /// Store two values to be compared in future.
    ///
    /// Expensive diffing is deferred until calling `Debug::fmt`.
    pub fn new(left: &'a TLeft, right: &'a TRight) -> StrComparison<'a, TLeft, TRight> {
        StrComparison { left, right }
    }
}

Do you see any problems with that approach?

@tommilligan
Copy link
Collaborator

The only potential issue I see is that one can instantiate StrComparison with arbitrary types and the compilation will only fail once trying to Display it, e.g.

Ah good spot. Yes, I think adding the bound to the constructor is sensible, if for some reason we want to relax it later we can make it more permissive.

If you'd like to add that and then squash everything together, I think we're good to go

As discussed in rust-pretty-assertions#24, pretty_assertion's `assert_eq` will escape line
breaks in multi-line strings, making diffs for them much harder to
read.

This commit introduces another variant, `assert_str_eq`, and the
associated `StrComparison`, which compares the raw strings, where
the newlines are not escaped.

Fixes rust-pretty-assertions#24

Co-authored-by: Tom Milligan <tom@reinfer.io>
@x3ro
Copy link
Contributor Author

x3ro commented Feb 1, 2022

Done!

@codecov-commenter
Copy link

Codecov Report

Merging #88 (e244a90) into main (88cd1be) will decrease coverage by 0.52%.
The diff coverage is 91.66%.

❗ Current head e244a90 differs from pull request most recent head 5271543. Consider uploading reports for the commit 5271543 to get more accurate results
Impacted file tree graph

@@            Coverage Diff             @@
##             main      #88      +/-   ##
==========================================
- Coverage   96.35%   95.83%   -0.53%     
==========================================
  Files           3        3              
  Lines         192      216      +24     
==========================================
+ Hits          185      207      +22     
- Misses          7        9       +2     
Impacted Files Coverage Δ
pretty_assertions/tests/macros.rs 95.71% <90.00%> (-2.29%) ⬇️
pretty_assertions/src/lib.rs 100.00% <100.00%> (ø)

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 88cd1be...5271543. Read the comment docs.

@tommilligan tommilligan merged commit a108544 into rust-pretty-assertions:main Feb 1, 2022
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.

Nicer formatting for multiline strings
3 participants