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

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

Closed

Conversation

tommilligan
Copy link
Collaborator

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

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 tommilligan self-assigned this May 5, 2021
@tommilligan tommilligan force-pushed the proc_macro branch 2 times, most recently from ee2026b to 41cd694 Compare May 6, 2021 08:19
@codecov-commenter
Copy link

codecov-commenter commented May 6, 2021

Codecov Report

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

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

@tommilligan tommilligan force-pushed the proc_macro branch 2 times, most recently from c7ba10f to 87b4081 Compare May 6, 2021 10:01
@colin-kiegel
Copy link
Collaborator

@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
                    )
                }
            }
        }
    });
}

@tommilligan
Copy link
Collaborator Author

Superseded by #116

@tommilligan tommilligan closed this May 7, 2023
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