Use namespace instead of class Approvals

[dimztimz]

All classes should be put inside namespace. And the class Approvals should be removed since it is just a collection of static functions. They should be free functions inside the namespace.

The name of the namespace should be ideally ApprovalTests. Downstream users would then use

ApprovalTests::verify(...);

[claremacrae]

Thank you for the recommendation.

If I understand correctly, all the classes and namespaces in ApprovalTests.cpp/ApprovalTests would go in a ApprovalTests namespace. I think that would help users with larger code bases, as we have some obvious names like SystemUtils and StringUtils which would cause problems if approvals, in its current form, were used in code-bases that already used these names.

Does that mean that CombinationApprovals::verifyAllCombinations() would become ApprovalTests::CombinationApprovals::verifyAllCombinations(). I presume so - I guess we would need to think about why the current class static-methods-only ApprovalTests would be any different from any of the other static-methods-only classes.

[claremacrae]

We are intending to introduce a consistent namespace to all of the contents of ApprovalTests/ soon - likely in the next week...

We may also add a second layer of namespaces, e.g. something like ApprovalTests::Writers::StringWriter...

Comments on this would be appreciated....

[claremacrae]

I've started experimenting with this in CLion, trying to find an efficient workflow for moving all our code in ApprovalTests/ to a new ApprovalTests namespace.

I'll record here what I've found - and ruled out.

[claremacrae]

Try 1

Just use Move refactoring in CLion - gets paths of headers wrong, so won’t compile - tried a script to remove the wrong lines - too much faff. Example build error:

In file included from /Users/clare/Documents/develop/ApprovalTests/ApprovalTests.cpp/tests/ApprovalTests_Catch2_Tests/CombinationTests.cpp:6:
In file included from /Users/clare/Documents/develop/ApprovalTests/ApprovalTests.cpp/ApprovalTests/../ApprovalTests/CombinationApprovals.h:7:
In file included from /Users/clare/Documents/develop/ApprovalTests/ApprovalTests.cpp/ApprovalTests/../ApprovalTests/reporters/DefaultReporter.h:5:
In file included from /Users/clare/Documents/develop/ApprovalTests/ApprovalTests.cpp/ApprovalTests/../ApprovalTests/reporters/DefaultReporterFactory.h:5:
In file included from /Users/clare/Documents/develop/ApprovalTests/ApprovalTests.cpp/ApprovalTests/../ApprovalTests/reporters/DiffReporter.h:5:
In file included from /Users/clare/Documents/develop/ApprovalTests/ApprovalTests.cpp/ApprovalTests/../ApprovalTests/reporters/WindowsReporters.h:4:
In file included from /Users/clare/Documents/develop/ApprovalTests/ApprovalTests.cpp/ApprovalTests/../ApprovalTests/reporters/DiffPrograms.h:4:
/Users/clare/Documents/develop/ApprovalTests/ApprovalTests.cpp/ApprovalTests/../ApprovalTests/reporters/DiffInfo.h:8:10: fatal error: 'FileUtils.h' file not found
#include "FileUtils.h"
         ^~~~~~~~~~~~~
In file included from /Users/clare/Documents/develop/ApprovalTests/ApprovalTests.cpp/tests/ApprovalTests_Catch2_Tests/ApprovalsTests.cpp:In file included from 4:

[claremacrae]

Try 2

In CLion, move all headers to top level of project, then use Move in CLion.
Too much faff - was quite time-consuming - and it made it harder to see the other changes.
Would also need another step to move all the files back to their original directory - and would mess up the history and Annotate/Blame too much.

[claremacrae]

Try 3

Go back to use Move refactoring in CLion - gets paths of headers wrong still, so work around that by editing CMakeLists.txt to temporarily get builds working with the new, wrong #include paths - with the intention to revert the CMake edit later, and delete all the wrong #includes in one go.

This was the first change in ApprovalTests/CMakeLists.txt - which was needed when refactoring headers in ApprovalTests - when we move on to the sub-directories, like namers, I expect we would need to make further edits to add those directories too:

-target_include_directories(${PROJECT_NAME}
-        INTERFACE ${CMAKE_CURRENT_SOURCE_DIR}/..)
+target_include_directories(${PROJECT_NAME}
+        INTERFACE
+        ${CMAKE_CURRENT_SOURCE_DIR}/..
+        # Temporary extra paths,
+        ${CMAKE_CURRENT_SOURCE_DIR}/
+        )

This went well, albeit slowly on a fast Mac, until I spotted that CLion was making needless edits to std::shared_ptr - changing it to std::__1::shared_ptr - same for std::vector.

These were hard to spot when viewing diffs, due CLion indenting the code that was moved to the namespace.

[claremacrae]

Try 4

Start again, as in previous step, but try to change CLion’s settings so that, for now, it doesn’t indent when adding namespace - so it should be easier to spot any extra edits that it makes during these refactoring steps.

[claremacrae]

Status

I ended up finishing this off by making the edits to headers by hand, then fixing build failures. It was faster than waiting for CLion's refactoring.

The changes on the namespaces_experiment branch show the progress I made on this.

(Click on the "Files Changed" tab to see the differences". I've minimised the changes, like not changing indentation, so that this work can be reviewed more easily.)

Basically, everything in the library is now in the namespace ApprovalTests. We talked about adding sub-namespaces like Reporters. My current thinking is to not do this, as it's likely too much work for what I understand the benefits to be.

I did this before rearranging the files in the library, to not get under the feet of someone who may possibly offer us some code improvements...

Remaining code to review

About the only files not fully updated are the Catch2 and Doctest integrations. We should discuss those.

Okra integration

I also spent about an hour looking at setting up a test for the Okra integration - and then found that it has not compiled for some time because of changes we made in ApprovalTests.cpp. But more significantly, it didn't build against the version of Okra.h from the time when this integrated in to this project.

I searched all of github for uses of the ApprovalTests.cpp/Okra integration, and there were none.

I've sent @JayBazuzi a message on Twitter to see if he wants to pair on it, but right now my feeling is we should probably just archive the Okra integration code, since no-one has logged a bug that it has been broken for some time.

Other Comments

It's possible that some of the ApprovalTests:: that were added early on could now be removed - they were added in library files that had not yet been moved to the namespace.

They are harmless, and I'm not worrying about the, now.

[claremacrae]

I've updated the docs too, on the branch, to give a sense what the code looks like now:

https://github.com/approvals/ApprovalTests.cpp/blob/2d99078f26049e8dfa6733dbee32e6fb637e1bec/doc/Namers.md

We might want to shorten lines in the first example in the above.

https://github.com/approvals/ApprovalTests.cpp/blob/2d99078f26049e8dfa6733dbee32e6fb637e1bec/doc/MultipleOutputFilesPerTest.md

The above does show the repetition - anyone who cares can do using namespace ApprovalTests; in their code - but it would trip people up if we did this in our docs, I feel.

[JayBazuzi]

I don't mind if you break Okra, because I have epsilon users.

One reason you might want to keep Okra working is that have Approval Tests work with multiple frameworks helps you know your integration points are sufficiently generally... But that's up to you.

[claremacrae]

Thanks Jay, the thing is more that, as far as I can tell, the ApprovalTests integration has never worked, and I can’t tell how to fix it.

[claremacrae]

This is now done.