Skip to content

Implementation of suggestions arising from pmslavin's review - #16

Open
OliverWoolland wants to merge 3 commits into
mainfrom
paul-review
Open

OliverWoolland wants to merge 3 commits into
mainfrom
paul-review

Conversation

@OliverWoolland

Copy link
Copy Markdown
Contributor

Discussion thread documenting suggested changes here: #9

Major changes:

  • cmake fetches catch2 automatically
  • read / write methods now go via nlohmann json to/from_json methods
  • error messages have been centralised
  • unordered_map has been used in place of map

Deferred:


Copilot summary

This pull request introduces several improvements and refactorings to the RO-Crate C++ library. The most significant changes include the introduction of a centralized error handling mechanism, improved error messages, and a switch to using unordered maps for better performance. Additionally, the CMake configuration has been updated to use a newer version and to fetch Catch2 via FetchContent for testing. The serialization and deserialization logic for RO-Crate entities has also been refactored for clarity and robustness.

Error handling improvements:

  • Introduced an ErrorCode enum and a centralized error_message function to standardize and clarify error messages throughout the codebase. All exceptions now use these error codes and message templates, often with std::format for dynamic values. [1] [2] [3] [4] [5] [6] [7]

Performance and data structure updates:

  • Replaced std::map with std::unordered_map for entity and property storage to improve lookup performance in Properties and EntityRegister. [1] [2]

Serialization and deserialization refactor:

  • Refactored the RO-Crate serialization and deserialization logic by introducing to_json and from_json functions, improving code clarity and separation of concerns. Error handling for file I/O and JSON parsing is now more robust and uses the new error handling system. [1] [2]

Build system improvements:

  • Updated the minimum required CMake version to 3.24 and switched Catch2 integration from find_package to FetchContent, ensuring consistent and up-to-date dependency management for tests. [1] [2]

These changes collectively improve maintainability, reliability, and performance of the RO-Crate library.

@OliverWoolland OliverWoolland self-assigned this Oct 12, 2026

@pmslavin pmslavin left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Builds and tests pass. Looks good overall; I've added a few minor comments which are mostly questions and clarifications.

Comment thread include/ro-crate.hpp
return "Failed to parse JSON from file: {}. Error: {}";
case ErrorCode::InvalidCrateStructure:
return "Invalid RO-Crate structure: missing '@context' or '@graph' fields.";
case ErrorCode::EntityMissingIdField:

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Is this different from ErrorCode::EntityMissingId on L55 above?

Comment thread include/ro-crate.hpp
updateProperties(entity, entityJson);
entity = crate.getEntity(entityId);
crate.updateProperties(entity, entityJson);
} catch (const std::runtime_error&) {

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

std::runtime_error is a broad category of exception to catch here; is there a narrower exception that more closely corresponds to the Entity not existing but excludes the various other situations that can give rise to std::runtime_error?

Or can the presence of the Entity in the register be tested directly without requiring try/catch?

Comment thread CMakeLists.txt
FetchContent_Declare(Catch2
GIT_REPOSITORY https://github.com/catchorg/Catch2.git
GIT_TAG v3.16.0
FIND_PACKAGE_ARGS 3

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Not important and probably harmless either way, but does FIND_PACKAGE_ARGS do anything here when an explicit GIT_TAG has been provided?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants