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.

Sign up for GitHub

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

Jump to bottom

Implement assert a file matches a fixture on disk #43

Merged
merged 1 commit into from
May 29, 2018
Merged

Implement assert a file matches a fixture on disk #43

merged 1 commit into from
May 29, 2018

Conversation

farodin91
Copy link
Contributor

@farodin91 farodin91 commented May 29, 2018 •
edited
Loading

Partial fixes #32

epage
epage requested changes May 29, 2018
View reviewed changes
src/path/ft.rs Outdated
@@ -59,6 +72,17 @@ impl FileTypePredicate {
self.follow = yes;
self
}

pub(crate) fn new(path: &path::Path) -> io::Result<FileTypePredicate> {
Copy link
Contributor

Choose a reason for hiding this comment

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

Would it be useful to make this pub?

What about naming it from_path or with_path?

src/path/ft.rs Outdated
})
}

pub(crate) fn is_file(&self) -> bool {
Copy link
Contributor

Choose a reason for hiding this comment

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

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

src/path/fs.rs Outdated

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

Choose a reason for hiding this comment

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

Please address unwrap

src/path/fs.rs Outdated

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

Choose a reason for hiding this comment

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

  1. this should be var is {}
  2. Should we print content or path?

src/path/fs.rs Outdated

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

Choose a reason for hiding this comment

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

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?

src/path/fs.rs Outdated
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) {
Copy link
Contributor

Choose a reason for hiding this comment

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

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

src/path/fs.rs Outdated
if !self.file_type.eval(path) {
false
} else {
self.file_content.clone().unwrap() == FileContent::new(path).unwrap()
Copy link
Contributor

Choose a reason for hiding this comment

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

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?

src/path/fs.rs Outdated

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

Choose a reason for hiding this comment

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

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

src/path/fs.rs Outdated
/// 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 {
Copy link
Contributor

Choose a reason for hiding this comment

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

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

@farodin91 farodin91 force-pushed the assert-fs-fixture branch 2 times, most recently from d223bb0 to 9c62111 Compare May 29, 2018 17:35
@farodin91 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
@farodin91
Copy link
Contributor Author

I integrated all changes.

@epage
Copy link
Contributor

epage commented May 29, 2018

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
epage approved these changes May 29, 2018
View reviewed changes
Copy link
Contributor

@epage epage left a comment

Choose a reason for hiding this comment

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

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.

src/prelude.rs Outdated
@@ -41,6 +41,8 @@ pub mod predicate {
///
/// This module contains predicates specific to path handling.
pub mod path {
pub use path::eq_file;
pub use path::FileTypePredicate;
Copy link
Contributor

Choose a reason for hiding this comment

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

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.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Was it because FileTypePredicate::from_path? Correct!

src/path/mod.rs Outdated
@@ -16,3 +16,5 @@ mod ft;
pub use self::ft::{is_dir, is_file, is_symlink, FileTypePredicate};
mod fc;
pub use self::fc::{FileContentPredicate, PredicateFileContentExt};
mod fs;
pub use self::fs::eq_file;
Copy link
Contributor

Choose a reason for hiding this comment

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

This should publicly expose BinaryFilePredicate and StrFilePredicate

@farodin91 farodin91 force-pushed the assert-fs-fixture branch from 9c62111 to 4d0569f Compare May 29, 2018 18:25
@farodin91 farodin91 force-pushed the assert-fs-fixture branch from 4d0569f to a69f72f Compare May 29, 2018 18:49
@farodin91
Copy link
Contributor Author

Fixed both request and the clippy failure

@epage epage merged commit 5beb0de into assert-rs:master May 29, 2018
@farodin91 farodin91 deleted the assert-fs-fixture branch May 29, 2018 20:45
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@epage epage epage approved these changes

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

Assert a file matches a fixture on disk
2 participants
@farodin91 @epage