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

feat: Add str::contains_all function #147

Open
wants to merge 2 commits into
base: master
Choose a base branch
from

Conversation

Techassi
Copy link

@Techassi Techassi commented Jul 16, 2023

This PR adds the str::contains_all predicate function. It allows one to check if the input contains all provided needles.

use predicates::prelude::*;

let predicate_fn = predicate::str::contains_all(vec!["One", "Two", "Three"]);

assert_eq!(true, predicate_fn.eval("One Two Three"));
assert_eq!(false, predicate_fn.eval("One Two Four"));
assert_eq!(false, predicate_fn.eval("Four Five Six"));

@mre
Copy link

mre commented Jul 17, 2023

In your example, shouldn't it be a vec with multiple strings as predicates, or am I misunderstanding something?

let predicate_fn = predicate::str::contains_all(vec!["One", "Two", "Three"]);

@Techassi
Copy link
Author

Techassi commented Jul 17, 2023

You are absolutely correct. I somehow forgot to add the commas.

EDIT: I added the commas in my comment above and in the doc test in 45d4a17.

Comment on lines +334 to +338
/// let predicate_fn = predicate::str::contains_all(vec!["One", "Two", "Three"]);
/// assert_eq!(true, predicate_fn.eval("One Two Three"));
/// assert_eq!(false, predicate_fn.eval("One Two Four"));
/// assert_eq!(false, predicate_fn.eval("Four Five Six"));
/// ```
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Whats the use case for this vs just doing multiple contains checks?

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Using multiple contains for a small number of strings to check is fine. This, however, becomes cumbersome when dealing with 10+ different strings to check. Here it would be neat to be able to check a list of strings, either coming from a file or hardcoded ones in the test case.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

In a way, this is a special casing of other predicates (anding a bunch of contains) and I'm trying to understand when we are justified in baking that in vs having people build it up from other parts.

Speaking of, I wonder if we should implement Predicate for tuples for easier and-ing.

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Speaking of, I wonder if we should implement Predicate for tuples for easier and-ing.

Yes, that could work. However, this again breaks when the user reaches N, with N being the maximum number of items in a tuple we implemented Predicate for.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Tuples can be composed of tuples.

And for a case like this, the types will all be the same, so we could also support arrays

@epage
Copy link
Contributor

epage commented Jul 17, 2023

If we move forward with this, can you squash your commits?

@Techassi
Copy link
Author

Yes, I can do that once we are ready with this PR.

Comment on lines +301 to +309
fn eval(&self, variable: &str) -> bool {
for pattern in &self.patterns {
if !variable.contains(pattern) {
return false;
}
}

true
}
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

If we do this, we should probably have a find_case implementation to either report which items matched so people don't have to manually scan the string for them

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Okay, I will look into this!

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I added a basic find_case impl. I'm unsure if the current impl does what this function expects. This is because I'm not quite sure how it integrates into the predicates-rs ecosystem.

But, the implementation can be improved by implementing reflection::PredicateReflection and fmt::Display for ContainsAllPredicate.

@epage
Copy link
Contributor

epage commented Jul 17, 2023

Yes, I can do that once we are ready with this PR.

The downside to that is it requires an extra ping-pong between us to get this in, causing more context switches, and being fairly easy to fall through the cracks

@Techassi
Copy link
Author

The downside to that is it requires an extra ping-pong between us to get this in, causing more context switches, and being fairly easy to fall through the cracks

Can't we just use squash-merge? Then GitHub would do the heavy lifting for us :)

@epage
Copy link
Contributor

epage commented Jul 18, 2023

Can't we just use squash-merge? Then GitHub would do the heavy lifting for us :)

Not the most ideal experience and I don't like blindly squashing PRs but only doing it as a last resort because we should preserve multiple commits when they are relevant.

@Techassi
Copy link
Author

Techassi commented Sep 9, 2023

I now squashed all previous commits (Adding the function, updating the docs, and merging changes from main).

I will now continue to work on the unresolved conversations above.

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.

3 participants