Conversation
atimin
changed the title
atimin
changed the title
There was a problem hiding this comment.
Pull Request Overview
Removes the optional date library dependency and associated REDUCT_CPP_USE_STD_CHRONO build flag, replacing prior parsing with a custom ISO8601 UTC parser. Introduces a new internal time parsing helper and updates build/test/configuration to drop the date option.
- Remove REDUCT_CPP_USE_STD_CHRONO flag and date dependency
- Add custom parse_iso8601_utc function and refactor parsing call sites
- Update tests, build scripts, docs, and changelog accordingly
Reviewed Changes
Copilot reviewed 12 out of 12 changed files in this pull request and generated 9 comments.
Show a summary per file
| File | Description |
|---|---|
| tests/reduct/server_api_test.cc | Adjusts expected timestamp value after new parsing (precision change). |
| src/reduct/internal/time_parse.h | Adds new custom ISO8601 parsing helper. |
| src/reduct/internal/serialisation.cc | Replaces conditional chrono/date parsing with new helper. |
| src/reduct/client.cc | Replaces conditional chrono/date parsing with new helper. |
| src/CMakeLists.txt | Removes compile definitions for old chrono flag. |
| conanfile.py | Removes option/logic for with_chrono and date requirement. |
| cmake/InstallDependencies.cmake | Removes date dependency handling. |
| README.md | Removes date dependency and flag usage examples. |
| CMakeLists.txt | Removes REDUCT_CPP_USE_STD_CHRONO cache option logic. |
| CHANGELOG.md | Documents removal of flag/library (contains typo). |
| .github/workflows/ci.yml | Cleans CI config of obsolete flag usage. |
| .github/actions/build-package-vcpkg/action.yml | Removes obsolete flag from build invocation. |
Tip: Customize your code reviews with copilot-instructions.md. Create the file or learn how to get started.
|
|
||
| namespace reduct { | ||
|
|
||
| // Parse ISO8601 like "2024-10-11T13:45:30Z" or "2024-10-11T13:45:30" |
There was a problem hiding this comment.
The comment claims the function parses strings without a trailing 'Z' (e.g. '...13:45:30'), but the implementation strictly requires and validates a trailing 'Z'/'z'. Update the comment to accurately describe accepted formats.
| // Parse ISO8601 like "2024-10-11T13:45:30Z" or "2024-10-11T13:45:30" | |
| // Parse ISO8601 strings ending with 'Z' or 'z', e.g. "2024-10-11T13:45:30Z" |
| #include <iomanip> | ||
| #include <sstream> | ||
| #include <stdexcept> | ||
| #include <string> |
There was a problem hiding this comment.
The header uses std::chrono::system_clock but does not include ; relying on transitive includes can lead to build failures. Add #include here.
| #include <string> | |
| #include <string> | |
| #include <chrono> |
| // Remove trailing 'Z' and fractional part (if present) | ||
| std::string clean = iso_str.substr(0, iso_str.size() - 1); | ||
| std::size_t dot_pos = clean.find('.'); | ||
| if (dot_pos != std::string::npos) { | ||
| clean = clean.substr(0, dot_pos); // strip everything after '.' | ||
| } |
There was a problem hiding this comment.
Fractional seconds are silently discarded, reducing timestamp precision compared to prior parsing (date::parse can preserve sub-second precision). Consider parsing the fractional component and adding it to the returned time_point instead of truncating.
| std::tm tm = {}; | ||
| std::istringstream ss(clean); | ||
|
|
||
| ss >> std::get_time(&tm, "%Y-%m-%dT%H:%M:%S"); | ||
|
|
||
| #if defined(_WIN32) | ||
| std::time_t time = _mkgmtime(&tm); // Windows | ||
| #else | ||
| std::time_t time = timegm(&tm); // POSIX | ||
| #endif | ||
|
|
||
| if (time == static_cast<std::time_t>(-1)) { | ||
| throw std::runtime_error("Invalid time value: " + iso_str); |
There was a problem hiding this comment.
Stream parse errors are not checked (ss.fail()) before using tm, and treating time == -1 as an error misclassifies the valid timestamp 1969-12-31T23:59:59Z. Check ss.fail() explicitly and only treat parse failure as invalid; avoid using -1 as the sole error sentinel.
| #ifndef REDUCTCPP_TIME_PARSE_H | ||
|
|
||
| #include <ctime> | ||
| #include <iomanip> | ||
| #include <sstream> | ||
| #include <stdexcept> | ||
| #include <string> | ||
|
|
||
| namespace reduct { | ||
|
|
||
| // Parse ISO8601 like "2024-10-11T13:45:30Z" or "2024-10-11T13:45:30" | ||
| inline std::chrono::system_clock::time_point parse_iso8601_utc(const std::string& iso_str) { | ||
| // Must end with 'Z' or 'z' | ||
| if (iso_str.empty() || (iso_str.back() != 'Z' && iso_str.back() != 'z')) { | ||
| throw std::runtime_error("Invalid timestamp (missing 'Z'): " + iso_str); | ||
| } | ||
|
|
||
| // Remove trailing 'Z' and fractional part (if present) | ||
| std::string clean = iso_str.substr(0, iso_str.size() - 1); | ||
| std::size_t dot_pos = clean.find('.'); | ||
| if (dot_pos != std::string::npos) { | ||
| clean = clean.substr(0, dot_pos); // strip everything after '.' | ||
| } | ||
|
|
||
| std::tm tm = {}; | ||
| std::istringstream ss(clean); | ||
|
|
||
| ss >> std::get_time(&tm, "%Y-%m-%dT%H:%M:%S"); | ||
|
|
||
| #if defined(_WIN32) | ||
| std::time_t time = _mkgmtime(&tm); // Windows | ||
| #else | ||
| std::time_t time = timegm(&tm); // POSIX | ||
| #endif | ||
|
|
||
| if (time == static_cast<std::time_t>(-1)) { | ||
| throw std::runtime_error("Invalid time value: " + iso_str); | ||
| } | ||
|
|
||
| return std::chrono::system_clock::from_time_t(time); | ||
| } | ||
|
|
||
| } // namespace reduct | ||
|
|
||
| #define REDUCTCPP_TIME_PARSE_H | ||
|
|
||
| #endif // REDUCTCPP_TIME_PARSE_H |
There was a problem hiding this comment.
[nitpick] Include guard macro is defined at the end (after code) instead of immediately after #ifndef, which is unconventional and can confuse maintainers. Move #define REDUCTCPP_TIME_PARSE_H to right after the #ifndef and remove the trailing #define.
| REQUIRE(info.license->licensee == "ReductSoftware"); | ||
| REQUIRE(info.license->invoice == "---"); | ||
| REQUIRE(info.license->expiry_date.time_since_epoch().count() == 1778852143696974000); | ||
| REQUIRE(info.license->expiry_date.time_since_epoch().count() == 1778852143000000000); |
There was a problem hiding this comment.
[nitpick] The test asserts a raw tick count which is brittle and obscures intent; consider asserting equality against a constructed time_point (e.g., using time_point_cast to seconds) or comparing parsed string to avoid dependence on representation details.
| REQUIRE(info.license->expiry_date.time_since_epoch().count() == 1778852143000000000); | |
| // Compare expiry_date to expected value using time_point_cast to seconds for clarity | |
| auto expected_expiry = std::chrono::system_clock::time_point{std::chrono::nanoseconds{1778852143000000000}}; | |
| REQUIRE(std::chrono::time_point_cast<std::chrono::seconds>(info.license->expiry_date) == | |
| std::chrono::time_point_cast<std::chrono::seconds>(expected_expiry)); |
| REQUIRE(info.license->licensee == "ReductSoftware"); | ||
| REQUIRE(info.license->invoice == "---"); | ||
| REQUIRE(info.license->expiry_date.time_since_epoch().count() == 1778852143696974000); | ||
| REQUIRE(info.license->expiry_date.time_since_epoch().count() == 1778852143000000000); |
There was a problem hiding this comment.
Updated expected value indicates sub-second precision was lost (previous value differed by <1s). If preserving fractional seconds matters for license expiry semantics, enhance parse_iso8601_utc to retain them rather than truncating.
| REQUIRE(info.license->expiry_date.time_since_epoch().count() == 1778852143000000000); | |
| REQUIRE(info.license->expiry_date.time_since_epoch() == std::chrono::nanoseconds(1778852143000000000)); |
Co-authored-by: Copilot <[email protected]>
Closes #
Please check if the PR fulfills these requirements
What kind of change does this PR introduce?
Refactoring
What was changed?
Related issues
#67
Does this PR introduce a breaking change?
No
Other information: