Implement assert a file matches a fixture on disk by farodin91 · Pull Request #43 · assert-rs/predicates-rs

farodin91 · 2018-05-29T14:15:16Z

Partial fixes #32

epage · 2018-05-29T14:23:37Z

src/path/ft.rs

        self
    }
+
+    pub(crate) fn new(path: &path::Path) -> io::Result<FileTypePredicate> {


Would it be useful to make this pub?

What about naming it from_path or with_path?

epage · 2018-05-29T14:23:58Z

src/path/ft.rs

+        })
+    }
+
+    pub(crate) fn is_file(&self) -> bool {


I think I'd rather we just return the file type.

epage · 2018-05-29T14:24:46Z

src/path/fs.rs

+
+impl Predicate<path::Path> for StrFilePredicate {
+    fn eval(&self, path: &path::Path) -> bool {
+        let content = FileContent::new(path).unwrap().utf8();


Please address unwrap

epage · 2018-05-29T14:25:19Z

src/path/fs.rs

+
+impl fmt::Display for StrFilePredicate {
+    fn fmt(&self, f: &mut fmt::Formatter) -> fmt::Result {
+        write!(f, "content is {}", self.content)


this should be var is {}

Should we print content or path?

epage · 2018-05-29T14:26:33Z

src/path/fs.rs

+
+/// Predicate that compares file matches
+#[derive(Clone, Debug)]
+pub struct FileSystemEntryPredicate {


Should we make this only work with files? We can handle dirs like we handle symlinks or non-existent files

If we keep it handling dirs, maybe shorten the name to EntryPredicate?

epage · 2018-05-29T14:27:35Z

src/path/fs.rs

+impl Predicate<path::Path> for FileSystemEntryPredicate {
+    fn eval(&self, path: &path::Path) -> bool {
+        if self.file_type.is_file() {
+            if !self.file_type.eval(path) {


I feel like it might be simpler if we just reused FileType and not FileTypePredicate

epage · 2018-05-29T14:29:26Z

src/path/fs.rs

+            if !self.file_type.eval(path) {
+                false
+            } else {
+                self.file_content.clone().unwrap() == FileContent::new(path).unwrap()


self.file_content.as_ref().unwrap() addresses the clone

The first unwrap should probably be an expect so you can document your assumption of why it is always safe to unwrap in this context.

Should the second unwrap instead have us return false?

epage · 2018-05-29T14:30:10Z

src/path/fs.rs

+
+impl fmt::Display for FileSystemEntryPredicate {
+    fn fmt(&self, f: &mut fmt::Formatter) -> fmt::Result {
+        write!(f, "var is {}", self.file_type)


Should it be file_type or the path? The downside being we'd have to store off the path

epage · 2018-05-29T14:30:44Z

src/path/fs.rs

+/// assert_eq!(false, predicate_dir.eval(Path::new("Cargo.toml")));
+/// assert_eq!(true, predicate_dir.eval(Path::new("src")));
+/// ```
+pub fn eq_file_system_entry(path: &path::Path) -> FileSystemEntryPredicate {


Again, we can shorten this to eq_entry if it accepts multiple types f eq_file if it only works with files

farodin91 · 2018-05-29T17:36:39Z

I integrated all changes.

epage · 2018-05-29T17:58:19Z

For background, we decided to simplify the EntryPredcate to BinaryFilePrediate.

If someone wants to compare a dir, they can use is_dir.
If someone wants to compare a file, they can use eq_file
That effectively covers 100% of the use case of a predicate that combines the two.

epage

Some minor clean up that I thought I'd point out since you have to go back and address clippy.

If you already have addressed clippy. oh well, this PR can move forward and it can be cleaned up later.

epage · 2018-05-29T17:59:06Z

src/prelude.rs

    /// This module contains predicates specific to path handling.
    pub mod path {
+        pub use path::eq_file;
+        pub use path::FileTypePredicate;


Why was FileTypePredicate added to the prelude?

The goal has been just to expose the minimum for creating predicates and not for general operation of the crate.

Was it because FileTypePredicate::from_path? I'd say remove the predicate from the prelude and lets worry later about how we want it exposed in to the user.

Was it because FileTypePredicate::from_path? Correct!

epage · 2018-05-29T18:01:36Z

src/path/mod.rs

 mod fc;
 pub use self::fc::{FileContentPredicate, PredicateFileContentExt};
+mod fs;
+pub use self::fs::eq_file;


This should publicly expose BinaryFilePredicate and StrFilePredicate

farodin91 · 2018-05-29T18:50:07Z

Fixed both request and the clippy failure

Implement assert a file matches a fixture on disk

epage requested changes May 29, 2018

View reviewed changes

farodin91 force-pushed the assert-fs-fixture branch 2 times, most recently from d223bb0 to 9c62111 Compare May 29, 2018 17:35

farodin91 changed the title ~~WIP: Implement assert a file matches a fixture on disk~~ Implement assert a file matches a fixture on disk May 29, 2018

epage approved these changes May 29, 2018

View reviewed changes

farodin91 force-pushed the assert-fs-fixture branch from 9c62111 to 4d0569f Compare May 29, 2018 18:25

Implement assert a file matches a fixture on disk

a69f72f

farodin91 force-pushed the assert-fs-fixture branch from 4d0569f to a69f72f Compare May 29, 2018 18:49

epage merged commit 5beb0de into assert-rs:master May 29, 2018

farodin91 deleted the assert-fs-fixture branch May 29, 2018 20:45

takumi-earth pushed a commit to earthlings-dev/predicates-rs that referenced this pull request Jan 27, 2026

Merge pull request assert-rs#43 from farodin91/assert-fs-fixture

010c5a9

Implement assert a file matches a fixture on disk

Conversation

farodin91 commented May 29, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

farodin91 commented May 29, 2018

Uh oh!

epage commented May 29, 2018

Uh oh!

epage left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

farodin91 commented May 29, 2018

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

2 participants

farodin91 commented May 29, 2018 •

edited

Loading