approvals / ApprovalTests.cpp

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

Enable those who clone this project to supply own Catch header #53

Closed claremacrae closed 4 years ago

claremacrae commented 4 years ago

People who download the single-header release of Approval Tests can use their own Catch header, so long as they name it Catch.hpp.

However, I was told at a recent C++ London Meetup that those download or fork this repo, and who include our top-level CMakeLists.txt file find that the Catch.hpp in our repo gets used in preference to theirs.

I was told that the workaround is to #include your own Catch header before pulling ours in...

Looking at that cmake file, I don't understand the cause of the probem, as I would have thought that our third_party directory was not added to the search path in that situation, due to these lines:


...
if(NOT DEFINED PROJECT_NAME)
    set(NOT_SUBPROJECT ON)
endif()

...

if (NOT_SUBPROJECT)
...
    add_subdirectory(third_party)
...
endif()

So I think we need more info about the problem in order to fix it...
claremacrae commented 4 years ago

Hallo @dmsteck - please excuse my memory, but was it you that requested this, and if so, could you add any more info to help us identify a solution please?

dmsteck commented 4 years ago

Hi @claremacrae, yep that was me. Thanks for remembering.

What you wrote in the OP is true to a certain extent.

However, the Catch2 integration header (ApprovalTests/integrations/catch/Catch2Approvals.h) includes Catch.hpp. Hence, to use the Catch2 integration, you actually need Catch.hpp on your include path. This leaves the user with two possibilities:

I'm not sure what an "elegant" solution would look like here. A simple suggestion that I can come up with would be to rename the Catch.hpp that ApprovalTests looks for to a lowercase catch.hpp, so that users can achieve the desired effect by linking against their own copy of Catch2.

PS: The above is only relevant when you import ApprovalTests.cpp by cloning the repo rather than using the single include. Based on your recent comments it seemed like you intended for people to use the single include?

claremacrae commented 4 years ago

Hi @dmsteck

That's really helpful - thank you. I'll discuss this with @isidore next time we pair...

I think the best approach would be to rename out Catch.hpp to catch.hpp so it's consistent with what anyone would download directly - I hadn't noticed that inconsistency. We'd want to include that the next time we bump the major version number, I think.

Re single include vs cloning... we originally only ever envisaged this would be used by people downloading the single header...

But we learned that it was going to be used by people cloning it when we had a bug report of pull request to fix our CMake files so that it would work this way - so it would be good to make sure it works well this way too.

Also, because creating the single header requires non-trivial dependencies, that's another reason why some who wanted to fork and modify code in this project would mostly likely include this repo directly - having them generate their own copy of the single header isn't a super-feasible thing to recommend...

claremacrae commented 4 years ago

This will be fixed in v7.0.0 - so closing now

claremacrae commented 4 years ago

Hi @dmsteck - just to let you know we released this tonight... https://github.com/approvals/ApprovalTests.cpp/releases/tag/v.7.0.0 I do hope it helps! Thanks for the feedback.

dmsteck commented 4 years ago

I approve! 😉