add inverse validation functions #58
Conversation
src/lib.rs
Outdated
| @@ -26,13 +26,13 @@ pub mod text; | |||
| #[derive(Clone, Debug)] | |||
| pub struct Validate<'a> { | |||
| /// Optionally validate the response status code. | |||
| status: Option<u16>, | |||
| status: Option<(bool, u16)>, | |||
There was a problem hiding this comment.
Cool addition! I have to say that I was quite confused when looking at this initially, because I expected the a true bool to indicate that it's required, while in reality the bool represents inverse.
I am thinking of what the alternatives would be:
- maybe having a struct with two fields: a bool and an arbitrary variable (that would make it a lot more explicit what the bool represents since it would have a name)
- having an enum with two options:
Required,Forbiddenand having data inside that
I'm not that good in Rust to be able to say if you can just wrap any kind of data in both of the options listed above :)
That's just my 2 cents.
There was a problem hiding this comment.
Or maybe we could keep the tuple and replace the bool with a new enum that has two values: Required, Forbidden. That would already be an improvement.
There was a problem hiding this comment.
This is an internal representation that is not exposed to the end-user. We could certainly flip the boolean (and then name it something other than inverse), or represent the information another way, but as this isn't exposed I'm not sure it's worth the effort?
As an end user, you'd call .text() to check if text exists in the page, or .not_text() to check that the text does not exist in the page, for example. (Before I merge, I plan to add some test coverage to confirm it's working as designed.)
Does that address your concerns, or you still feel the inverse variable is confusing?
There was a problem hiding this comment.
You're right that the final user will just have to pick between .text() and .not_text(), which is easy.
I saw that the struct is public and that mislead me.
There was a problem hiding this comment.
Ah, it almost certainly should not be a public struct.
There was a problem hiding this comment.
Note that all fields are private, not public: https://docs.rs/goose-eggs/latest/goose_eggs/struct.Validate.html
There was a problem hiding this comment.
@alecsmrekar I reworked the internal representation per your feedback
| @@ -598,11 +598,11 @@ pub async fn log_in( | |||
| }; | |||
|
|
|||
| // Load the log in page. | |||
| let goose = if validate.status.is_some() { | |||
| let goose = if let Some(validate_status) = validate.status.as_ref() { | |||
There was a problem hiding this comment.
Cool pattern, I haven't seen this before
not_status()to confirm status code is not returnednot_title()to confirm text is not in the title of the pagenot_text()andnot_texts()to confirm text is not on the pagenot_header()andnot_header_value()to confirm header and/or header value is not returned