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

Add diffToolBuilder for building complex diff command messages #840

Open
wants to merge 2 commits into
base: main
Choose a base branch
from

Conversation

js
Copy link

@js js commented Mar 23, 2024

This PR adds SnapshotTesting.diffToolBuilder closure for building complex diff tool command messages or clickable urls in Xcode. It takes the existing and the failure snapshot as arguments, and thus complex commands which are not just prefixed with the existing diffTool can be created.

For instance, to open Differati from a clickable link in Xcode the following diffToolBuilder closure can be provided, which URL encodes the paths (in case they contain spaces etc) and builds an url openable by the app:

SnapshotTesting.diffToolBuilder = { existing, failed in
    func encode(_ path: String) -> String {
        path.addingPercentEncoding(withAllowedCharacters: .urlPathAllowed) ?? path
    }
    return "differati://show?old=\(encode(existing.path))&new=\(encode(failed.path))"
}

Which gives us an Xcode test failure bubble like this, with a clickable url that takes us directly into the diffing app:
image

Or it can be used to build a complex diff command with the path arguments aren't simply a suffix, as requested in #780:

SnapshotTesting.diffToolBuilder = { "compare \"\($0.path)\" \"\($1.path)\" png: | open -f -a Preview.app" }

… messages

It takes the existing and the failure snapshot as arguments, and thus complex commands which are not just prefixed with the existing diffTool can be created.
@andrewjmeier
Copy link

Thank you for doing this!!!

let diffMessage =
diffTool
Copy link
Member

Choose a reason for hiding this comment

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

As it stands this PR will break existing use of diffTool. We're down to support this feature (as an alternative to #508), but can you make the following change?

  1. Update var diffTool to be a computed getter/setter to diffToolBuilder
  2. Consider some alternative names? Maybe diffToolCommand?

Copy link
Author

@js js Apr 6, 2024

Choose a reason for hiding this comment

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

Added commit with the suggested changes:

The existing diffTool always had precedence over the closure, but agreed a custom getter+setter makes the diffMessage site cleaner. However it wasn't obvious to me how to construct an empty URL for the diffTool getter so I changed the diffToolCommand closure to receive the path as String rather than the file URL.

Renamed it to diffToolCommand instead.

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