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

Better documentation for find_case #71

Open
asomers opened this issue Jan 22, 2019 · 8 comments
Open

Better documentation for find_case #71

asomers opened this issue Jan 22, 2019 · 8 comments

Comments

@asomers
Copy link
Contributor

asomers commented Jan 22, 2019

It sounds like Predicate::find_case is important, but for the life of me I can't figure out how to use it. Could you please expand its documentation? What I'm looking for is a way to do something like this, so I can print a useful message whenever a predicate fails.

assert!(pred.eval(&input), pred.fail_message(&input))
@epage
Copy link
Contributor

epage commented Jan 22, 2019

Yeah, this is an area that needs some work.

assert_fs and assert_cmd have some examples, for example: https://github.com/assert-rs/assert_cmd/blob/master/src/assert.rs#L255

Something else I've been meaning to do is write a hamcrest-like crate for so you can just do assert_that!(value, pred); and you get the information printed for you.

@epage
Copy link
Contributor

epage commented Jan 22, 2019

btw the .tree() is coming from the extension trait brought in at https://github.com/assert-rs/assert_cmd/blob/master/src/assert.rs#L12.

@ian-h-chamberlain
Copy link

@epage I have created a simple assert_that macro which does basically what you describe for use in my own project:

pub use predicates;
pub use predicates_tree;

#[macro_export]
macro_rules! assert_that {
    ($item:expr, $pred:expr $(,)?) => {{
        use $crate::predicates::prelude::*;
        use $crate::predicates_tree::CaseTreeExt;

        if let Some(case) = $pred.find_case(false, $item) {
            panic!("{}", case.tree());
        };
    }};
}

Is this good enough for a first pass at inclusion to the library? I'd be happy to adapt it and open a PR if so.

I'm not sure if this would be part of predicates-tree, or maybe an optional feature of this crate? It seems general-purpose enough to be part of this crate but obviously requires CaseTreeExt to work.

@epage
Copy link
Contributor

epage commented Jun 18, 2020

Is this good enough for a first pass at inclusion to the library? I'd be happy to adapt it and open a PR if so.

Sounds great, thanks!

I'm not sure if this would be part of predicates-tree, or maybe an optional feature of this crate? It seems general-purpose enough to be part of this crate but obviously requires CaseTreeExt to work.

Hmm, thats an interesting point.

It should either go in predicates or we should have a separate fluent-assertion crate.

  • I think the main question is if we'd want more than just assert_that. Is this a one-off convenience tool or should we have a separate crate that can evolve independently.
  • For predicates, we'd want to allow people to disable it so they don't have to pull in the tree rendering crate
  • A big challenge is that predicates and predicates_tree are different crates and either one needs to re-export the other or we need to require the user to add both to their Cargo.toml file.

Thoughts?

@ian-h-chamberlain
Copy link

Hmm, maybe a separate crate does make sense. Without it, users would need to either opt-in or opt-out of using this macro, since it creates a new dependency. Having a feature flag for a single convenience macro seems a little heavy-handed to me, but so does a new crate for that matter.

I think it is generally a somewhat common practice to have macro-only crates as conveniences for working with library crates, although in my experience those tend to be complex procedural macros which generate a lot of boilerplate code, rather than something simple like this.

For what it's worth, it seems the name assert_that is not yet taken on crates.io, so maybe that makes sense? It would have dependencies on both predicates and predicates-tree and re-export them for the sake of the macro, I suppose. I'm not sure it would be very ergonomic to use without also importing predicates::prelude::* since otherwise you have no predicate to assert on! Unless we used syntax like the following, assuming it is possible with macro namespacing rules:

extern crate assert_that;
use assert_that::assert_that;

#[test]
fn does_it_work() {
    // expands to use re-exported predicate, i.e. 
    // `assert_that::predicates::prelude::predicate::str::similar`
    assert_that!("Hello world", str::similar("Goodbye, world"));
}

I could see some additional extensions potentially being added to this, although I think most code changes would be made in the predicates-tree and predicates crates rather than in the implementation of this macro. For reference, I would look at GTest Matchers and their ASSERT_THAT macro for a possible UX for this one.

Perhaps future extensions could include helper macros for writing custom predicates ("matchers") like their MATCHER_P macro? Beyond that, I'm not sure what more could be done within that assertion crate in general, but I think there's a possibility for future enhancements.

Assuming that a separate crate makes sense – would you prefer to create a separate repo, or just a subcrate within this repo's Cargo workspace?

@epage
Copy link
Contributor

epage commented Jun 20, 2020

Assuming that a separate crate makes sense – would you prefer to create a separate repo, or just a subcrate within this repo's Cargo workspace?

Like with the other crates (assert_fs, assert_cmd), I'm thinking separate crate but can also live in this org.

@ian-h-chamberlain
Copy link

@epage I have created and published a 0.1 version of assert_that: https://github.com/ian-h-chamberlain/assert_that

I also created ian-h-chamberlain/assert_that#2 where we can discuss whether or not it makes sense to move it to this organization. I'm inclined to wait and see if the crate gets much/any usage or new use cases arise before transferring ownership, but if you'd rather transfer it sooner I think that would be fine.

@epage
Copy link
Contributor

epage commented Jun 23, 2020

Thanks! Excited to see this develop!

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

No branches or pull requests

3 participants