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

lib: add helpers for generating custom assert_{eq,ne} macros

Open tommilligan opened this issue 4 years ago • 2 comments

This PR uses proc macro magic to cut down the boilerplate needed to define macros that fit the assert_eq and assert_ne api. Our own assert_eq is now simply defined as:

pretty_assertions_derive::derive_assert_eq! {
    (assert_eq, |left_val, right_val, has_message, message| {
        ::std::panic!("assertion failed: `(left == right)`{}{}\
           \n\
           \n{}\
           \n",
           if has_message { ": " } else { "" },
           message,
           $crate::Comparison::new(left_val, right_val)
        )
    })

I'm going to leave this here for comment, as I'm not certain I want to merge this into pretty_assertions. I think it could feasibly be it's own crate and used in combination with pretty_assertions, for users who don't care abbout the additional dependency weight.

Pros

  • Nice and readable
  • Extensible to other assert_eq definitions (see https://github.com/colin-kiegel/rust-pretty-assertions/issues/24#issuecomment-821482452)

Cons

  • Increases dependency tree
    • proc-macro2
      • unicode-xid
    • quote
    • syn
  • Additional layer of complexity
    • Adds proc macros to the stack
    • Nested macro definitions!
  • Macros produced using this definition cannot be called within in the same crate.

Potential workarounds (dismissed)

  • Don't implement pretty_assertion's core implementations using these procedural macros.
    • This would cut dependencies, but lead to duplicate code (which would be a pain to maintain).
  • Try to implement matching functionality in plain macros.
    • I think this is theoretically possible but not fun.

tommilligan avatar May 05 '21 18:05 tommilligan

Codecov Report

Merging #79 (ece4160) into main (55f9b7a) will decrease coverage by 18.77%. The diff coverage is 10.90%.

:exclamation: Current head ece4160 differs from pull request most recent head ab6d726. Consider uploading reports for the commit ab6d726 to get more accurate results Impacted file tree graph

@@             Coverage Diff             @@
##             main      #79       +/-   ##
===========================================
- Coverage   96.95%   78.17%   -18.78%     
===========================================
  Files           4        6        +2     
  Lines         197      252       +55     
===========================================
+ Hits          191      197        +6     
- Misses          6       55       +49     
Impacted Files Coverage Δ
pretty_assertions/src/lib.rs 100.00% <ø> (ø)
pretty_assertions_derive/src/lib.rs 0.00% <0.00%> (ø)
pretty_assertions_derive_tests/tests/assert_eq.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 55f9b7a...ab6d726. Read the comment docs.

codecov-commenter avatar May 06 '21 08:05 codecov-commenter

@tommilligan Hey Tom, I came across this interesting PR. :-)

I can see how this allows for compact custom definitions of assert-macros. However I'm not yet sure if it is worth the effort to maintain a proc macro.

IMO

  • most use cases for customization really only care about the part $crate::Comparison::new(left_val, right_val). I think we should focus on how to allow customizations here
  • if you really want to have full control all details, it is not too hard to copy-paste-edit macro_rules! assert_eq { /* ... */ }

What do you think about something like this:

  • We define the assertion macros with subcalls to a helper macro pretty_comparison
  • The consuming crate can decide to either import our implementation of pretty_comparison - or make its own definition with or without using our building blocks

Maybe like this?

// HELPER MACRO
#[macro_export]
macro_rules! pretty_comparison {
    ($left:expr, $right:expr) => ({
        $crate::Comparison::new(left, right)
    });
}

#[macro_export]
macro_rules! assert_eq {
    ($left:expr, $right:expr$(,)?) => ({
        $crate::assert_eq!(@ $left, $right, "", "");
    });
    ($left:expr, $right:expr, $($arg:tt)*) => ({
        $crate::assert_eq!(@ $left, $right, ": ", $($arg)+);
    });
    (@ $left:expr, $right:expr, $maybe_semicolon:expr, $($arg:tt)*) => ({
        match (&($left), &($right)) {
            (left_val, right_val) => {
                if !(*left_val == *right_val) {
                    ::std::panic!("assertion failed: `(left == right)`{}{}\
                       \n\
                       \n{}\
                       \n",
                       $maybe_semicolon,
                       format_args!($($arg)*),
                       pretty_comparison!(left_val, right_val) // <-- SUBCALL TO A HELPER MACRO, WHICH CAN BE REDEFINED BY USERS
                    )
                }
            }
        }
    });
}

colin-kiegel avatar May 16 '21 08:05 colin-kiegel

Superseded by #116

tommilligan avatar May 07 '23 16:05 tommilligan