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
Open
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
4 changes: 4 additions & 0 deletions CHANGELOG.md
Original file line number Diff line number Diff line change
Expand Up @@ -7,6 +7,10 @@ and this project adheres to [Semantic Versioning](http://semver.org/).
<!-- next-header -->
## [Unreleased] - ReleaseDate

### Added

- Add `str::contains_all` function

## [3.0.3] - 2023-04-13

### Internal
Expand Down
2 changes: 2 additions & 0 deletions src/lib.rs
Original file line number Diff line number Diff line change
Expand Up @@ -143,6 +143,7 @@
//! - [`predicate::str::ends_with`]: Specified string must end with the given needle.
//! - [`predicate::str::contains`]: Specified string must contain the given needle.
//! - [`predicate::str::contains(...).count`]: Required number of times the needle must show up.
//! - [`predicate::str::contains_all`]: Specified string must contain all given needles.
//! - [`predicate::str::is_match`]: Specified string must match the given regex.
//! - [`predicate::str::is_match(...).count`]: Required number of times the match must show up.
//! - [`str_pred.trim`]: Trim whitespace before passing it to `str_pred`.
Expand Down Expand Up @@ -188,6 +189,7 @@
//! [`predicate::path::missing`]: prelude::predicate::path::missing()
//! [`predicate::str::contains(...).count`]: str::ContainsPredicate::count()
//! [`predicate::str::contains`]: prelude::predicate::str::contains()
//! [`predicate::str::contains_all`]: prelude::predicate::str::contains_all()
//! [`predicate::str::diff`]: prelude::predicate::str::diff()
//! [`predicate::str::ends_with`]: prelude::predicate::str::ends_with()
//! [`predicate::str::is_empty`]: prelude::predicate::str::is_empty()
Expand Down
2 changes: 1 addition & 1 deletion src/prelude.rs
Original file line number Diff line number Diff line change
Expand Up @@ -28,7 +28,7 @@ pub mod predicate {
/// This module contains predicates specific to string handling.
pub mod str {
pub use crate::str::is_empty;
pub use crate::str::{contains, ends_with, starts_with};
pub use crate::str::{contains, contains_all, ends_with, starts_with};

#[cfg(feature = "diff")]
pub use crate::str::diff;
Expand Down
78 changes: 78 additions & 0 deletions src/str/basics.rs
Original file line number Diff line number Diff line change
Expand Up @@ -288,3 +288,81 @@ where
pattern: pattern.into(),
}
}

/// Predicate that checks for all patterns.
///
/// This is created by `predicates::str:contains_all`.
#[derive(Debug, Clone, PartialEq, Eq)]
pub struct ContainsAllPredicate {
patterns: Vec<String>,
}

impl Predicate<str> for ContainsAllPredicate {
fn eval(&self, variable: &str) -> bool {
for pattern in &self.patterns {
if !variable.contains(pattern) {
return false;
}
}

true
}
Comment on lines +301 to +309
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.


fn find_case<'a>(&'a self, expected: bool, variable: &str) -> Option<reflection::Case<'a>> {
let mut missing = None;

for pattern in &self.patterns {
if !variable.contains(pattern) && !expected {
missing = Some(pattern)
}
}

match missing {
Some(m) => Some(
reflection::Case::new(Some(self), false)
.add_product(reflection::Product::new("missing", m.to_owned())),
),
None => None,
}
}
}

impl reflection::PredicateReflection for ContainsAllPredicate {}

impl fmt::Display for ContainsAllPredicate {
fn fmt(&self, f: &mut fmt::Formatter<'_>) -> fmt::Result {
let palette = crate::Palette::new(f.alternate());
write!(
f,
"{}.{}({})",
palette.var("var"),
palette.description("contains_all"),
palette.expected(format!("{:?}", &self.patterns)),
)
}
}

/// Creates a new `Predicate` that ensures a str contains `pattern`
///
/// # Examples
///
/// ```
/// 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"));
/// ```
Comment on lines +352 to +356
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

pub fn contains_all<P, T>(patterns: P) -> ContainsAllPredicate
where
P: IntoIterator<Item = T>,
T: AsRef<str>,
{
let patterns: Vec<_> = patterns
.into_iter()
.map(|p| p.as_ref().to_string())
.collect();

ContainsAllPredicate { patterns }
}