Skip to content
This repository has been archived by the owner on Dec 29, 2021. It is now read-only.

Output satisfies predicate? #55

Closed
dsprenkels opened this issue Oct 16, 2017 · 7 comments
Closed

Output satisfies predicate? #55

dsprenkels opened this issue Oct 16, 2017 · 7 comments

Comments

@dsprenkels
Copy link
Contributor

Thanks for this awesome library!

I would like to check if the command's output is from a specific format. I am specifically dealing with formats like this:

0156dc3c96b0939fa...
02fab783bdfc017dd...

with the first byte being a counted integer, and the rest random. So I would like to check if the binary lengths are correct and if the first byte is 01, 02, ....

Is it possible to pass a function to a OutputAssertionBuilder method which allows me to check by predicate?

If not, I would like to propose one (OutputAssertionBuilder::satisfy), which you can call with a function which checks if the string satisfies a given predicate.

Example type signature:

fn satisfies<F>(pred: F) -> Assert
    where F : FnOnce(&str) -> bool
@epage
Copy link
Collaborator

epage commented Oct 16, 2017

Currently, we only support exact matches and substring matches.

The idea is welcome. In #20, we talk about regex matches and fn matches. For an fn, we either need to Box it or have a state-less pointer.

Your suggestion for the name is satisfies. In the API, this would look like

fn all_lowercase(output: &str) -> bool {
   ...
}

...
    .stdout().satisfies(all_lowercase)
    .unwrap();

I don't (yet?) have much in the way of opinions on the name. Is there any kind of precedence that we should be consistent with?

@killercup
Copy link
Collaborator

killercup commented Oct 16, 2017 via email

@dsprenkels
Copy link
Contributor Author

dsprenkels commented Oct 16, 2017

Could it be something like this?

It changes the code style, but I think that the OutputAssertion would become a bit more flexible. There is however the issue of error reporting, which gets a little harder. For the error building, I was thinking about taking an extra field in the OutputAssertion struct containing an error builder. Another option could be for pred to return a Result and putting the error building in the assert.rs code. Then the user could call two functions like predicate (-> bool) or predicate_ok (-> Result), depending on whether they want to customize their error or something.

Or ditch this plan altogether, cause closures are not very pleasant to work with. 😉

@epage
Copy link
Collaborator

epage commented Oct 17, 2017

A lot of the value is the rich error reporting, we need to be sure to preserve that.

While this might not need Debug/Clone, I still hesitate when not supporting such basic traits.

Until someone has a use case for it, we could avoid unifying the code branches and have a stateless function pointer rather than a stateful one in a Box.

@dsprenkels
Copy link
Contributor Author

dsprenkels commented Oct 17, 2017

I restored the error reporting and restored the Debug/Clone trait implementations. Currently I use a Rc for the Clone functionality, which looks a bit ugly but is not really a problem (?). I don't really understand what you mean by your last statement, do you mean using &'a Fn everywhere, instead of a boxed type?

I also implemented the functions that I mentioned before (although only 'static, which I think should be enough). Shall I open a pull request?

(Compare changes: https://github.com/killercup/assert_cli/compare/master...dsprenkels:predicates?expand=1)

@epage
Copy link
Collaborator

epage commented Oct 18, 2017

Huh, somehow lost my post

I don't really understand what you mean by your last statement, do you mean using &'a Fn everywhere, instead of a boxed type?

I was thinking of a function pointer rather than a function trait unless there is a use case where we need the trait instead.

Shall I open a pull request?

Yes please, it'll provide a better forum for talking about the code.

Also, maybe I'm just being crotchety but I'm somewhat preferring the enum approach rather than unifying everything behind a boxed Fn trait. Thoughts?

@epage
Copy link
Collaborator

epage commented Nov 6, 2017

btw my current thought on what this API could look like

  • satisfies(Fn(String) -> bool, message: String)
    • message is there since we can't gleam anything from the Fn
  • satisfies_ok(Fn(String) -> Result<(), Box<Error>>)
    • Box<Error> so that the caller can provide a completely custom error message to explain why their assertion failed.

epage added a commit to epage/assert_cli that referenced this issue Feb 3, 2018
This accepts a message with it.  This should hit the 90% case of a
`satisfies_ok` (or whatever it'd be called).

I'm also assuming that it'll be a best practice to document the custom
predicates, so its acceptable to force it on everyone.

If a `satisfies_ok` is found to be needed, I'm assuming its because the
user wants to tie into existing machinery that has error reporting.  This
means we'll probably need to accept an `Fn` that `Box`es the error to
preserve it.

Fixes assert-rs#55
@epage epage closed this as completed in abe2606 Feb 4, 2018
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Projects
None yet
Development

No branches or pull requests

3 participants