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

Accept extra parameters in assert_eq!, pass them to panic! #1604

Closed
SimonSapin opened this issue May 1, 2016 · 6 comments
Closed

Accept extra parameters in assert_eq!, pass them to panic! #1604

SimonSapin opened this issue May 1, 2016 · 6 comments
Labels
T-libs-api Relevant to the library API team, which will review and decide on the RFC.

Comments

@SimonSapin
Copy link
Contributor

The assert! macro has a one-argument form that simply takes a boolean expression, and a "detailed" form where extra arguments are passed to panic!, allowing the user to change the panic message.

The assert_eq! macro however only has one form: two arguments that are compared for equality.

https://twitter.com/natpryce/status/656110689082286080?s=09

@ rustlang's assert_eq macro doesn't take a msg param, so I pass it tuples of values & context to get good diagnostics. What do others do?

I propose adding another form form assert_eq! where additional arguments are passed to panic!. But unlike assert!, I think this should add to the default message instead of replacing it. Something like:

macro_rules! assert_eq {
    ($left:expr , $right:expr) => ({
        match (&($left), &($right)) {
            (left_val, right_val) => {
                if !(*left_val == *right_val) {
                    panic!("assertion failed: `(left == right)` \
                           (left: `{:?}`, right: `{:?}`)", left_val, right_val)
                }
            }
        }
    });
    ($left:expr , $right:expr, $fmt:expr, $($arg:tt)*) => ({
        match (&($left), &($right)) {
            (left_val, right_val) => {
                if !(*left_val == *right_val) {
                    panic!(concat!("assertion failed: `(left == right)` \
                                   (left: `{:?}`, right: `{:?}`) ", $fmt),
                           left_val, right_val, $($arg)*)
                }
            }
        }
    });
}

CC @alexcrichton

@brendanzab
Copy link
Member

brendanzab commented May 1, 2016

Good idea. I would be more inclined toward making the whole format string customisable though:

assert_eq!(..., ..., "blah blah '{left}' '{right}' blah {}", some_value);
    ($left:expr , $right:expr, $fmt:expr, $($arg:tt)*) => ({
        match (&($left), &($right)) {
            (left_val, right_val) => {
                if !(*left_val == *right_val) {
                    panic!($fmt, left=left_val, right=right_val, $($arg)*)
                }
            }
        }
    });

@ticki
Copy link
Contributor

ticki commented May 2, 2016

This has always been strange, especially considering that without this functionality, it could be a normal function instead.

@SimonSapin
Copy link
Contributor Author

@bjz I proposed concatenating instead of replacing the format string based on the assumption that, if you use assert_eq! instead of assert! with == you always want to print/format the two values.

@ticki With a function instead of a macro, the potentially-expensive string formatting would have to be done even when it’s not necessary (when the two values are equal and the assertion succeeds).

@ticki
Copy link
Contributor

ticki commented May 2, 2016

With a function instead of a macro, the potentially-expensive string formatting would have to be done even when it’s not necessary (when the two values are equal and the assertion succeeds).

Oh, don't get me wrong. I simple state that, as it is currently, having it as a macro is unnecessary since there is no formatting involved.

@jonas-schievink
Copy link
Contributor

jonas-schievink commented Jul 19, 2016

rust-lang/rust#33976 was merged, can this be closed?

@alexcrichton
Copy link
Member

Indeed!

@Centril Centril added the T-libs-api Relevant to the library API team, which will review and decide on the RFC. label Feb 23, 2018
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
T-libs-api Relevant to the library API team, which will review and decide on the RFC.
Projects
None yet
Development

No branches or pull requests

6 participants