Skip to content

Add in diffToolSuffix to support ImageMagick cli #668

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

Closed
wants to merge 10 commits into from

Conversation

tisohjung
Copy link

@tisohjung tisohjung commented Oct 26, 2022

Added diffMessageConverter to support other tools like ImageMagick
which has commands that looks like magick compare "${image1}" "${image2}" diffImage.png

    diffMessageConverter = { "magick compare \"\($0)\" \"\($1)\" diff.png ; open diff.png" }

@tisohjung tisohjung force-pushed the feature/diff-tool-suffix branch from 2d1ca9c to 459cb43 Compare December 8, 2022 14:50
@mlostekk
Copy link

any idea when this will be merged?

Comment on lines 9 to 10
public var diffTool: String? = nil
public var diffToolSuffix: String = ""
Copy link

Choose a reason for hiding this comment

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

What about rework these two into a reusable, tool-independent message converter?
I'm suggesting this due to the following. Although, the solution offered works fine with these 2 tools, but what about using separate switches to prefix each item (for example: another_img_diff_tool -d 42 -i img1.png -i img2.png?

Something like this (at the moment I have not bothered to deprecate the old variables, but that would complete the picture):

public var diffMessageConverter: (String, String)  String? = { "\(diffTool) \($0) \($1)" }

Copy link
Author

Choose a reason for hiding this comment

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

good suggestion
8959584

@tisohjung tisohjung requested a review from chosa91 June 6, 2024 09:14
@stephencelis
Copy link
Member

Closed by #840.

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.

6 participants