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: control filenames in DiffOptions #36

Merged
merged 2 commits into from
Jan 29, 2025
Merged

Conversation

eholk
Copy link
Contributor

@eholk eholk commented Jan 28, 2025

I'm using diffy to format output in a test running, to make it easier to find where our actual output differed from the expected output. As a small quality of life improvement, it'd be nice if the diffs said "expected" and "actual" rather than "original" and "modified".

This PR adds options to DiffOptions to be able to explicitly set the filenames.

src/diff/mod.rs Outdated
@@ -46,6 +46,8 @@ where
pub struct DiffOptions {
compact: bool,
context_len: usize,
original_filename: Option<String>,
Copy link
Contributor Author

Choose a reason for hiding this comment

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

I think it'd be better or more general to use Option<Cow<'a, str>> here. That'd be a semver-breaking change on DiffOptions so I went with just a String instead.

If you're willing to take a semver break, I'd be happy to add a <'a> to DiffOptions and use Cow<'a, str> here instead. That'd also simplify the changes I made in create_patch.

Copy link
Owner

Choose a reason for hiding this comment

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

As a compromise we can use a Cow<'static, str> to avoid adding the lifetime to DiffOptions for now.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Ah, good call. Thanks!

@bmwill
Copy link
Owner

bmwill commented Jan 29, 2025

@eholk thanks for the PR! This has been a feature requested in the past so i suppose its time to add it :)

@bmwill bmwill merged commit 26322ac into bmwill:master Jan 29, 2025
2 checks passed
@bmwill
Copy link
Owner

bmwill commented Jan 29, 2025

I'll try to make another release with this change by the end of the day.

@eholk
Copy link
Contributor Author

eholk commented Jan 29, 2025

Thanks for taking the change and for the quick response!

@bmwill bmwill mentioned this pull request Jan 29, 2025
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.

2 participants