Observation and Discussion from Review

[pmslavin]

This issue is to collect observations and comments on the ro-crate-cpp library and to provide a forum for discussing these.

[pmslavin]

CMake FetchContent Module

CMake contains a FetchContent Module which allows requirements to be automatically retrieved and made available to the build procedure.

To apply this to Catch2, the CMakeLists.txt should include....

if(BUILD_TESTING)
    include(FetchContent)

A definition of the source and version for Catch2 can then be added with FetchContent_Declare()...

    FetchContent_Declare(Catch2 
        GIT_REPOSITORY https://github.com/catchorg/Catch2.git
        GIT_TAG        v3.16.0
    )

...and then the package retrieved with FetchContent_MakeAvailable()...

    FetchContent_MakeAvailable(Catch2)

This has the advantage of not requiring "FindCatch2.cmake" in CMAKE_MODULE_PATH or requiring the user to point to a Catch2Config.cmake file.

[pmslavin]

Error strings as static members

There are a few occasions where the text of error messages is repeated, such as:

ro-crate-cpp/include/ro-crate.hpp

Line 103 in 5cc0ebf

throw std::invalid_argument("Property name cannot be empty.");

...and...

ro-crate-cpp/include/ro-crate.hpp

Line 117 in 5cc0ebf

throw std::invalid_argument("Property name cannot be empty.");

...and other instances where the same error is repeated for different types (here Property and Value)

ro-crate-cpp/include/ro-crate.hpp

Lines 102 to 105 in 5cc0ebf

	if (property.empty())
	throw std::invalid_argument("Property name cannot be empty.");
	if (value.empty())
	throw std::invalid_argument("Property value cannot be empty.");

This doesn't represent much of a problem while the library is small, but such messages are likely to proliferate as the library grows and will require being kept in sync when changes are made.

An alternative is to have a single template form of such strings as a static class member...

class Entity{
    private:
        static constexpr std::string_view err_propempty{"Property {} cannot be empty"};
};

This can then be consumed where required using std::format to customise the message:

if (property.empty()){
    throw std::invalid_argument(std::format(err_propempty, "name"));
}

This requires only a single string to maintain for all errors of a particular type, and having a level of indirection between the exception ctor and the string makes localisation/translation much easier. A more advanced (extreme..?) version of this could have string templates held in a std::map member and then looked up dynamically.