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

Allow slashes in diff paths with looks-same #276

Conversation

kurttomlinson
Copy link
Contributor

This PR is a partial fix for #229. It creates the output diff folder immediately before asking looks-same to create a diff file. Because it only creates the folder when a diff file is going to be created, it does not create any empty folders.

This patch only applies to the looks-same differ. The Graphics Magic differ creates diff files even when there are no differences, so it needs to be fixed in a different way.

Copy link
Collaborator

@techeverri techeverri left a comment

Choose a reason for hiding this comment

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

I think a better approach would be to write the diff to a temp dir and then move it if there is a diff. Otherwise we'd end up with a bunch of empty directories.

Based on the comment above #229 (comment) by @oblador I would say this is not the desired behavior

@techeverri
Copy link
Collaborator

techeverri commented Oct 10, 2020

I would recommend you to use the workaround #229 (comment) suggested by @SebDuf using patch-package in #229

@kurttomlinson
Copy link
Contributor Author

@techeverri I don’t think that comment applies in this case. There is an if (isSame) case above that handles when the images are the same. This newly added line of code is only executed when we already know there is a difference and a diff file needs to be created. This PR will not create any unnecessary folders.

@techeverri
Copy link
Collaborator

You're right @kurttomlinson 👍

@techeverri
Copy link
Collaborator

It would be very useful to have some kind of automated test for this fix 🤔

I'm thinking of an example project configured to have custom fileNameFormatter that uses / slashes (subfolders)

@techeverri
Copy link
Collaborator

BTW because of the way GitHub Actions work builds won't run when they come from forked repos (this pull request for example)
Recently I learned we shouldn't merge without running the changes through the CI pipeline

@kurttomlinson
Copy link
Contributor Author

@techeverri I've added a spec to cover this change.

@kurttomlinson
Copy link
Contributor Author

@techeverri Sorry to be a bother, but is there something I could do to help you review and merge this PR? 🙏

@oblador
Copy link
Owner

oblador commented Nov 24, 2020

@kurttomlinson I've fixed so that tests run properly now 👍

@oblador oblador merged commit 711e1e7 into oblador:master Nov 24, 2020
@kurttomlinson kurttomlinson deleted the allow-slashes-in-diff-paths-with-looks-same branch November 24, 2020 21:10
@kurttomlinson
Copy link
Contributor Author

Thanks for the fix and the merge @oblador ! 🎉

@oblador
Copy link
Owner

oblador commented Nov 24, 2020

Thanks for the PR! I was having similar thoughts actually :-D

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.

None yet

3 participants