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

Expose a diffing iterator #17

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

Expose a diffing iterator #17

wants to merge 7 commits into from

Conversation

epage
Copy link
Collaborator

@epage epage commented Feb 2, 2018

Some example use cases

  • Report more detailed information than is_different
    • first diff
    • all diffs
    • all regular and diffs
  • Custom content comparisons
    • line-ending agnostic
    • Using the difference crate

You can look at the implementation of is_different to see how simple it is. I have a test branch of cobalt that replaces the custom directory diffing with this, augmented to use difference:

fn assert_dirs_eq(expected: &Path, actual: &Path) {
     for entry in dir_diff::DirDiff::new(expected, actual) {
        let entry = entry
            .unwrap()
            .assert_exists()
            .and_then(dir_diff::DiffEntry::assert_file_type)
            .and_then(assert_content);
        if let Err(error) = entry {
            panic!("{}", error);
        }
      }
  }

Controversial API choices

  • Prefixing the test functions with assert despite them erroring rather than panicing. I wasn't sure of a better name

Potential areas for the API to expand

  • Make it even easier to get the first or all differences
  • Make it easier to have a nice panic

Potential areas for chrome improvement

  • AssertionError's Display
  • Have a fancy feature that uses difference
    • Limit on file size?
    • Assume anything convertable to String / UTF-8 is text?
    • Use a extension crate to detect text?
    • Use a content sniffer to detect text?

epage added 7 commits January 27, 2018 16:09
This is a step towards publicly exposing it.

We do lose the optimization from the last major PR.  We should still be
better than the original code though.
This is more so making room for other kinds of errors that will be
added.

BREAKING CHANGE: `is_different` now returns `IoError` instead of
`Error`.
Fixes #11 by giving users access to more detailed error information if
they use `DirDiff`'s `IntoIter` instead of `is_different`.

This makes it possible to workaround several existing issues:
- #9 by providing your own line-ending agnostic checks
- #6 by providing a file comparison function that handles larger files.
@epage
Copy link
Collaborator Author

epage commented Feb 2, 2018

@steveklabnik This is a big change. Is it something you're wanting to look at before I submit?

}

/// Returns an error iff one of the two paths does not exist.
pub fn assert_exists(self) -> Result<Self, AssertionError> {
Copy link
Collaborator Author

Choose a reason for hiding this comment

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

These asserts are a it of a pain to work with. Until the other day, I didn't know you could have more complex selfs.

This might be a way to improve things.

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.

1 participant