Variants of the `expect_*` methods that don't panic (i.e., return `Result`)

[d4h0]

Hi,

I just discovered asserthttp, and this looks pretty useful!

Have you thought about adding variants of the expect_* methods that don't panic (i.e., return Result) ? This would make this crate usable for code outside of tests.

[beltram]

Hi, and thanks for the kind words :)

To be honest I have never considered using this crate outside tests. It allowed to never consider optimizing anything (especially when it comes to body assertions, it sometimes has to clone bytes because reading takes ownership).

We could however try to add some try_expect_* variants, gated by a fallible feature to achieve this. This won't be optimal at first (performance wise) but it could help.

What is your usecase by the way ? Which http client are you using and does performance matters to you or not ?

[d4h0]

To be honest I have never considered using this crate outside tests. It allowed to never consider optimizing anything (especially when it comes to body assertions, it sometimes has to clone bytes because reading takes ownership).

I think, maximum performance is not important for some/many use-cases. For example, I'm building a tool using reqwest that notifies me if some events happen in some of my accounts (of websites that don't have good notification systems).

I'm interested in asserthttp to, basically, notice failures early, and display better error messages. The methods of asserthttp are kind of a DSL to check HTTP things, which would improve readability, I believe.

We could however try to add some try_expect_* variants

try_expect_* would be okay, but I think something short would be better... :)

Wouldn't is_* be good as well?

It seems, in most cases (if not all), asserthttp's methods should anyway return a boolean (not Result, as I've mentioned above).

gated by a fallible feature to achieve this.

I haven't looked at the source code, but wouldn't it be possible to just implement the existing expect_* methods with those is_* methods?

[beltram]

Hey, so I'll do this in 2 steps...

First has been done in #196, which lets you name your methods whatever you like. Which is better than enforcing a name for all.

I'll then introduce a fallible later on

[d4h0]

Thanks for your work, @beltram!

which lets you name your methods whatever you like. Which is better than enforcing a name for all.

With this, we'd basically create aliases for asserhttp's trait manually, right?

Personally, I'd rather use methods with longer names, than creating aliases myself, to be honest. I just mentioned shorter names because this can't be changed later.

Btw., I thought a bit about how I'd implement a library like asserhttp for maximum usability, and I came up with the following:

#[derive(Debug, thiserror::Error)]
pub enum Error {
    #[error("Status wasn't okay")]
    ResponseStatus,
    // Variants for errors of other checks
}

pub trait Asserhttp {
    fn is_status_ok(&self) -> bool {
        // Example for `reqwest`
        self.response().status().is_success() // or whatever
    }

    fn expect_status_ok(&self) {
        if !self.is_status_ok() {
            panic!("Status wasn't okay")
        }
    }

    fn try_expect_status_ok(&self) -> Result<(), Error> {
        // Ideally, with a shorter "naming formula", but I can't come up with anything shorter
        if self.is_status_ok() {
            Ok(())
        } else {
            Err(Error::ResponseStatus)
        }
    }

}

Basically, expect_status_ok would be good for tests and “script-like” code where panicking is okay.

try_expect_status_ok would be good for regular code where panicking would not be good, but an ergonomic way to check for certain things is desired (i.e., in combination with ?).

is_status_ok would implement the actual logic that checks things, and therefore would be the base of expect_status_ok and try_expect_status_ok (maybe expect_status_ok can be implemented with try_expect_status_ok, but that probably wouldn't have any significant benefits).

is_status_ok also could be used in tests, “script-like” code, and other code for implementing checks where more flexibility in handling the unexpected state is required.

For example, asserhttp has expect_status_ok, but not expect_status_not_ok. With is_status_ok something like expect_status_not_ok can be implemented easily withing a test or other code.

is_*, expect_*, and try_expect_* should not be too hard to generate via a declarative macro:

generate!([
    (
        is_status_ok,
        expect_status_ok,
        try_expect_status_ok,
        "Status wasn't okay",
        ResponseStatus,
        self.response().status().is_success()
    ),
    // ...
])

Feel free to ignore the above, but the approach seems to make sense to me.

[beltram]

Hey, thanks for the detailed suggestion, however:

With this, we'd basically create aliases for asserhttp's trait manually, right?

Kinda yes. You actually extend the base trait + a blanket implementation does the trick.

Personally, I'd rather use methods with longer names, than creating aliases myself, to be honest

Designing an API is hard and everyone has its say about it. My main concern is not to bloat the API. That's why I'll hide the fallible API behind a feature flag. Adding in addition the is_* variants will double the API surface, not to mention these are not fluent to begin with which was one of the crate's selling point compared to just using the http client native assertion methods.

For example, asserhttp has expect_status_ok, but not expect_status_not_ok

In conclusion, for these corner cases I tried to minimize the API surface by allowing closures. So your example can be written like this: .expect_status(|s| assert_ne!(s, 200))

[d4h0]

Okay, sounds good. I think, I might not understand a few things correctly, so I'll just wait and have a look again when everything is finished.

[beltram]

Fixed in #220