search

approvals / ApprovalTests.cpp

Native ApprovalTests for C++ on Linux, Mac and Windows
https://approvaltestscpp.readthedocs.io/en/latest/
Apache License 2.0
316 stars 51 forks source link

Add Support for SublimeMerge. Fixes #102 #103

Closed jwillikers closed 4 years ago

jwillikers commented 4 years ago

Description

Fixes #102. ApprovalTests can now use the SublimeMerge tool on Linux, macOS, and Windows.

Solution

I followed the instructions at on how to add new merge tools from here. To support multiple versions of SublimeMerge on Linux, I added extra DiffInfo's and Reporter classes with appropriate suffixes to distinguish between them. This is not ideal because it adds clutter and more boilerplate, but I think this can be fixed better in a future pull request meant to simplify how merge tools are added.

Handling Multiple Installations on Linux

On Linux, there is additional support for the SublimeMerge Snap, Flatpak, and tarball installations. Other info about calling SublimeMerge from the command-line can be found here. In the case of multiple installation on Linux, the one chosen is determined as follows:

  1. Snap
  2. Flatpak
  3. Any tool on the path named "smerge"
  4. The installed tarball

Please note that while the Flatpak package is official, the Snap package is unofficial at this time.

Remaining Work

The markdown snippets and single-header file will need to be regenerated. I haven't got to this yet because it is a bit of pain when it comes to getting the dependencies on Ubuntu.

I have verified the merge tool is called correctly on macOS, Windows, and on Linux for the Snap package. I should probably test for the other alternatives on Linux as well.

jwillikers commented 4 years ago

I just verified that in addition to the Snap, the Flatpak, tarball, and installation from Sublime's apt repository work. 👍

claremacrae commented 4 years ago

I just verified that in addition to the Snap, the Flatpak, tarball, and installation from Sublime's apt repository work. 👍

Hi @jwillikers, this is excellent! Thank you for a good and thorough job, well done. It will prompt me to try out Sublime Merge, too - and I will improve our docs on adding reporters, to cover the FirstWorkingReporter too.

I’ll regenerate the docs and then release it soon ... 🎉 Do you have a Twitter account, for credit? If so, what’s the handle, please?

claremacrae commented 4 years ago

Whilst running the tests on my machine, I just noticed the double quotes at the start and end of this line... These are the first reporters in this file to run in the foreground, so it's the first time I've seen this output...

Naively, I would have thought that the pairs of double-quotes would have confused the CMD exe - but as you've tested it on Windows, I guess it somehow just magically works....

I realised that even if this was a real problem, it was independent of this PR, whose changes are obviously correct...

So I've gone ahead and merged this PR - separation of concerns, and all that...

Thanks again, Jordan!

jwillikers commented 4 years ago

I just verified that in addition to the Snap, the Flatpak, tarball, and installation from Sublime's apt repository work. 👍

Hi @jwillikers, this is excellent! Thank you for a good and thorough job, well done. It will prompt me to try out Sublime Merge, too - and I will improve our docs on adding reporters, to cover the FirstWorkingReporter too.

I’ll regenerate the docs and then release it soon ... 🎉 Do you have a Twitter account, for credit? If so, what’s the handle, please?

Thank you, you're too kind. However, I don't have a Twitter account. 🤗