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 Sublime Merge #102

Closed claremacrae closed 4 years ago

claremacrae commented 4 years ago

This requires #98 to be implemented first.

For required command-line arguments, see #43 - which are used in https://github.com/approvals/ApprovalTests.cpp/blob/master/doc/how_tos/UseACustomReporter.md#top

claremacrae commented 4 years ago

This would be trivial to implement now... all the pieces are in place.

claremacrae commented 4 years ago

The DiffInfo args are now honoured... the new code would look something like this - but with the arguments parameters added...:

namespace Mac {
APPROVAL_TESTS_MACROS_ENTRY(SUBLIME_MERGE,
                            DiffInfo("/Applications/Sublime Merge.app/Contents/SharedSupport/bin/smerge", Type::TEXT))
}

namespace Linux {
APPROVAL_TESTS_MACROS_ENTRY(SUBLIME_MERGE, DiffInfo("smerge", Type::TEXT))
}

namespace Windows {
APPROVAL_TESTS_MACROS_ENTRY(SUBLIME_MERGE, DiffInfo("{ProgramFiles}Sublime Merge\\smerge.exe", Type::TEXT))
}

....

        class SublimeMergeReporter : public GenericDiffReporter
        {
        public:
            SublimeMergeReporter()
                : GenericDiffReporter(DiffPrograms::Mac::SUBLIME_MERGE())
            {
                launcher.setForeground(true);
            }
        };

Note the call to launcher.setForeground(true); - which prevents the command from being launched in the background...

claremacrae commented 4 years ago

@jwillikers I'd really appreciate a Pull Request, if you were able to test the above - assuming it makes sense...

claremacrae commented 4 years ago

See also https://github.com/approvals/ApprovalTests.cpp/blob/master/doc/how_tos/SubmitANewReporterToApprovalTests.md#top

claremacrae commented 4 years ago

Entirely optional, of course.... 😀

jwillikers commented 4 years ago

@claremacrae I'd love too! I will later today. The only caveat I have to add is that the Snap version will use the name sublime_merge and the Flatpak com.sublimemerge.App. I also know that Snaps run on macOS as well as Linux.

claremacrae commented 4 years ago

Thanks @jwillikers ! 🙏

I’m not familiar with Snap and Flatpak - but definitely don’t feel constrained to stick with the current layers of implementation if it is easier to assemble the info differently, to support multiple possible paths.

Also, if for example you have multiple different paths to specify, maybe FirstWorkingReporter could gain a new constructor that took a container of DiffInfos... just a wild idea... no idea if you need it!