Add specialized predicates #18

epage · 2018-04-01T04:37:09Z

All external crates are optional
Specialized functions are in a child module with a name for the type.
- Names would be verbose anyways
- This provides the user the opportunity to shorten it if they desire.

Particular notes

Edit-distance is limited in use until An eval that tells me why the predicate failed #7 is implemented. Its still nice to have now to help ensure An eval that tells me why the predicate failed #7 supports everything it needs
I'm a little mixed on the regex API. I'm starting to think its sufficient as is but we should probably document it to clarify how to work around the constrained by straight forward API.

epage · 2018-04-01T04:45:27Z

I think the main remaining predicates that assert_cli will want are

Creating a predicate from a file
A dir-diff predicate

After that, I'm going to explore ergonomics

EqPredicate stores T and its eval takes a &T. But what about Vec/slice, String/str, and PathBuf/path?
Adapters from one Item type to another, like wrapping a predicate on str with a predicate on [u8] that will do a from_utf8 on the variable before passing it down.

nastevens

There are some things I'd like to see addressed before merging, but this is seriously good stuff.

nastevens · 2018-04-02T02:50:19Z

src/predicate/float/close.rs

@@ -0,0 +1,101 @@
+use float_cmp::ApproxEq;


New files need a copyright notice - I recommend the following:

// Copyright (c) 2018 The predicates-rs Project Developers. // // Licensed under the Apache License, Version 2.0 <LICENSE-APACHE or // http://www.apache.org/licenses/LICENSE-2.0> or the MIT license // <LICENSE-MIT or http://opensource.org/licenses/MIT>, at your // option. This file may not be copied, modified, or distributed // except according to those terms.

I will also plan to go back and modify the notices that list me as the copyright holder and replace with the statement above.

I personally rely on the project-wide license rather than per file.

If you prefer pre-file, we should make sure to get a CONTRIBUTING that mentions that.

The recommendation of the FSF (via the GPLv3) is to include the license statement in all source files, and having dealt with way more FOSS licensing BS than I care to think about, I just feel better having the license in all files to avoid any confusion.

I've opened #22 and #23 to track the other changes needed so the whole project is licensed to the "predicates-rs Project Developers" and to make sure we get a CONTRIBUTING document. For this PR I think just adding the statement above is sufficient.

nastevens · 2018-04-02T02:52:20Z

src/predicate/path/existence.rs

+
+/// Creates a new `Predicate` that ensures the path doesn't exist.
+///
+///


Extra newline here

nastevens · 2018-04-02T02:56:57Z

src/predicate/path/ft.rs

+        };
+        metadata
+            .map(|m| self.ft.eval(&m.file_type()))
+            .unwrap_or_default()


Would be more readable with .unwrap_or(false) - it took me three clicks to recognize that this is the default -> for type boolean -> which means false because false is the default for boolean.

In cases like this, I'm on the fence and I just happened to lean the other way :)

nastevens · 2018-04-02T02:58:54Z

src/predicate/path/ft.rs

+///
+/// # Examples
+///
+/// ```ignore


Since all the tests should be false, wouldn't it be okay to un-ignore this doctest?

I guess it still has some value even if it isn't actually testing true for anything.

nastevens · 2018-04-02T03:01:20Z

src/predicate/str/difference.rs

+    /// - `"\n" for line-level.
+    ///
+    /// Default: `"\n"`
+    pub fn split<S>(mut self, split: S) -> Self


Instead of forcing a potential allocation, this could use the copy-on-write Cow type

I'm always on the fence on allocation vs indirection (Cow). I'll go ahead and switch it to Cow.

nastevens · 2018-04-02T03:06:21Z

src/predicate/str/difference.rs

+/// let predicate_fn = str::diff("Hello World");
+/// assert_eq!(true, predicate_fn.eval("Hello World"));
+/// ```
+pub fn diff<S>(orig: S) -> DifferencePredicate


The behavior of this predicate seems backwards to me based on the name. diff to me sounds like the predicate should be satisfied (return true) when the strings are different. But the behavior seems to be that the result is true when the strings are similar. Is there a better name that could be used for this - maybe like distance?

While this has features for edit-distance, as a user, my real care about is showing the diff between the strings once #7 is available and I was going for a name that communicated that.

I can see how the name implies the opposite meaning. What if instead I reversed the behavior of eval instead? The downside is that almost everyone will now need to not it.

Would it make sense to create two different predicates that handle the two different cases (looking to see if strings differ, looking to see if strings are the same)? The underlying type could easily be shared between two facades with clear naming.

Eh, sure. Updated.

nastevens · 2018-04-02T03:09:04Z

src/predicate/str/regex.rs

+    }
+}
+
+/// Creates a new `Predicate` that diffs two strings.


Should actually be something like "applies a regular expression match to a string"

nastevens · 2018-04-02T03:19:33Z

EqPredicate stores T and its eval takes a &T. But what about Vec/slice, String/str, and PathBuf/path?

The Predicate trait could be changed to accept Borrow<T> instead of &T. Since Borrow<T> is implemented for &T, this probably won't even end up being a breaking change, and would allow things using String and Vec to be way more ergonomic.

epage · 2018-04-02T13:32:03Z

Just force-pushed a fix for everything except diff.

nastevens · 2018-04-04T03:21:30Z

Looks like you picked up some stray commits (8c292d1 and 8afa6ae) during the last rebase - those commits already exist on master.

This will be more helpful when assert-rs#7 is implemented. Fixes assert-rs#9

This is a short term API. Different issues we'll need to consider when evolving this include: - Easy to create predicate - The ability to customize the regex (e.g. case sensitivity, multi-line, ignoring whityespace). - It looks like the flags can be enabled with `(?flag)` inside the regex. Good enough? - How error handling should work. Re-export? A single error type for the whole crate? This is a part of assert-rs#12

Part of assert-rs#8.

Fixes assert-rs#11

epage · 2018-04-04T12:57:26Z

Fixed

Can't believe I forgot to rebase master after rebase -i.

epage · 2018-04-06T01:13:32Z

FYI This should now be ready to go

(I would have merged it already but you've set up the repo to require approval from a reviewer and I've wanted to respect that and not just disable it)

nastevens

Found some typos - I'll leave it up to you if you want to fix before merging or just tack them onto a different PR. I don't want to hold you up more than I already have.

nastevens · 2018-04-11T01:32:46Z

src/predicate/float/mod.rs

+
+//! Float Predicates
+//!
+//! This module contains predicates specifiuc to string handling.


typo: "specifiuc"

nastevens · 2018-04-11T01:33:18Z

src/predicate/float/close.rs

+impl IsClosePredicate {
+    /// Set the amount of error allowed.
+    ///
+    /// Values `1`-`5` should work in most cases.  Some times more control is needed and you will


typo: "Some times" -> "Sometimes"

nastevens · 2018-04-11T01:34:46Z

src/predicate/path/mod.rs

+
+//! Path Predicates
+//!
+//! This module contains predicates specifiuc to the file system.


typo: specifiuc

nastevens · 2018-04-11T01:35:52Z

src/predicate/str/mod.rs

+
+//! String Predicates
+//!
+//! This module contains predicates specifiuc to string handling.


Another instance of this specifiuc copy-paste ;-)

epage requested a review from nastevens April 1, 2018 04:37

epage force-pushed the pred branch from 65de224 to afe93b0 Compare April 1, 2018 04:51

nastevens reviewed Apr 2, 2018

View reviewed changes

epage force-pushed the pred branch from afe93b0 to f2b8fca Compare April 2, 2018 13:31

nastevens mentioned this pull request Apr 2, 2018

API doesn't work well with distinct owned / ref types #20

Closed

epage force-pushed the pred branch from f2b8fca to 210b6fd Compare April 2, 2018 23:20

epage mentioned this pull request Apr 3, 2018

Internal DSL for Predicates? #19

Open

epage added 6 commits April 4, 2018 06:56

feat(str): Edit-distance predicate

84eb9fb

This will be more helpful when assert-rs#7 is implemented. Fixes assert-rs#9

feat(path): Existence predicate

ed21e85

Part of assert-rs#8.

feat(path): File type predicates

7f35fd3

Part of assert-rs#8.

feat(float): is_close Predicate

6b646af

Fixes assert-rs#11

feat(CI): Ensure compiles without features

fc9521a

epage force-pushed the pred branch from 210b6fd to fc9521a Compare April 4, 2018 12:56

This was referenced Apr 7, 2018

Regex matching. assert-rs/assert_cli#85

Closed

Regex matching in OutputAssertionBuilder assert-rs/assert_cli#81

Closed

nastevens previously approved these changes Apr 11, 2018

View reviewed changes

chore: Fix typos

0a6d632

epage dismissed nastevens’s stale review via 0a6d632 April 11, 2018 14:16

epage merged commit 083fe5d into assert-rs:master Apr 11, 2018

epage deleted the pred branch April 11, 2018 14:38

This was referenced Apr 16, 2018

Filesystem predicates #8

Closed

Take inspiration from hamcrest's predicates #12

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add specialized predicates #18

Add specialized predicates #18

epage commented Apr 1, 2018

epage commented Apr 1, 2018

nastevens left a comment

nastevens Apr 2, 2018

epage Apr 2, 2018

nastevens Apr 2, 2018

nastevens Apr 2, 2018

nastevens Apr 2, 2018

epage Apr 2, 2018

nastevens Apr 2, 2018

epage Apr 2, 2018

nastevens Apr 2, 2018

epage Apr 2, 2018

nastevens Apr 2, 2018

epage Apr 2, 2018

nastevens Apr 2, 2018

epage Apr 2, 2018

nastevens Apr 2, 2018

nastevens commented Apr 2, 2018

epage commented Apr 2, 2018

nastevens commented Apr 4, 2018

epage commented Apr 4, 2018

epage commented Apr 6, 2018

nastevens left a comment

nastevens Apr 11, 2018

nastevens Apr 11, 2018

nastevens Apr 11, 2018

nastevens Apr 11, 2018

Add specialized predicates #18

Add specialized predicates #18

Conversation

epage commented Apr 1, 2018

epage commented Apr 1, 2018

nastevens left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

nastevens commented Apr 2, 2018

epage commented Apr 2, 2018

nastevens commented Apr 4, 2018

epage commented Apr 4, 2018

epage commented Apr 6, 2018

nastevens left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment