feat(clp-s): Implement part of the parsing side of the new timestamp_parser utility.
#1385
Conversation
…several compilation errors.
WalkthroughAdds a new clp_s::timestamp_parser component: CMake target, build-option wiring, headers, implementation, error-code types, and unit tests; implements parse_timestamp to parse patterned timestamps into epoch nanoseconds and return explicit error codes. Changes
Sequence Diagram(s)sequenceDiagram
autonumber
participant Test as Unit Tests
participant Parser as clp_s::timestamp_parser::parse_timestamp
participant Helpers as Internal helpers
participant DateLib as date::date / chrono
participant Err as ystdlib::error_handling
Test->>Parser: parse_timestamp(timestamp, pattern, generated_pattern)
Parser->>Helpers: scan pattern, handle escapes, match literals/specifiers
alt Unsupported specifier
Parser-->>Err: return FormatSpecifierNotImplemented
Parser-->>Test: Result<Error>
else Parsed components
Parser->>DateLib: validate year/month/day & compute weekday (if provided)
alt Invalid date or mismatch
Parser-->>Err: return InvalidDate / IncompatibleTimestampPattern / InvalidTimestampPattern
Parser-->>Test: Result<Error>
else Valid date/time
Parser->>DateLib: compute epoch nanoseconds
Parser-->>Test: Result<Success>(epochtime_ns, used_pattern_view)
end
end
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes
Pre-merge checks and finishing touches❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
✨ Finishing touches🧪 Generate unit tests (beta)
📜 Recent review detailsConfiguration used: CodeRabbit UI Review profile: ASSERTIVE Plan: Pro 📒 Files selected for processing (1)
🧰 Additional context used🧠 Learnings (1)📚 Learning: 2025-08-09T04:07:27.083ZApplied to files:
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (15)
🔇 Additional comments (2)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 6
📜 Review details
Configuration used: CodeRabbit UI
Review profile: ASSERTIVE
Plan: Pro
📒 Files selected for processing (9)
components/core/CMakeLists.txt(2 hunks)components/core/cmake/Options/options.cmake(3 hunks)components/core/src/clp_s/CMakeLists.txt(1 hunks)components/core/src/clp_s/timestamp_parser/CMakeLists.txt(1 hunks)components/core/src/clp_s/timestamp_parser/ErrorCode.cpp(1 hunks)components/core/src/clp_s/timestamp_parser/ErrorCode.hpp(1 hunks)components/core/src/clp_s/timestamp_parser/TimestampParser.cpp(1 hunks)components/core/src/clp_s/timestamp_parser/TimestampParser.hpp(1 hunks)components/core/src/clp_s/timestamp_parser/test/test_TimestampParser.cpp(1 hunks)
🧰 Additional context used
📓 Path-based instructions (1)
**/*.{cpp,hpp,java,js,jsx,tpp,ts,tsx}
⚙️ CodeRabbit configuration file
- Prefer
false == <expression>rather than!<expression>.
Files:
components/core/src/clp_s/timestamp_parser/test/test_TimestampParser.cppcomponents/core/src/clp_s/timestamp_parser/ErrorCode.cppcomponents/core/src/clp_s/timestamp_parser/TimestampParser.hppcomponents/core/src/clp_s/timestamp_parser/ErrorCode.hppcomponents/core/src/clp_s/timestamp_parser/TimestampParser.cpp
🧠 Learnings (3)
📚 Learning: 2024-11-26T19:12:09.102Z
Learnt from: LinZhihao-723
PR: y-scope/clp#486
File: components/core/src/clp/error_handling/ErrorCode.hpp:131-134
Timestamp: 2024-11-26T19:12:09.102Z
Learning: In the error handling system (`components/core/src/clp/error_handling/ErrorCode.hpp`), the `ErrorCode` class is intentionally designed to wrap the `ErrorCodeEnum`, not to use the enum directly. This wrapper provides a general interface for adding customized error enums.
Applied to files:
components/core/src/clp_s/timestamp_parser/ErrorCode.cppcomponents/core/src/clp_s/timestamp_parser/ErrorCode.hpp
📚 Learning: 2024-11-26T19:16:19.933Z
Learnt from: LinZhihao-723
PR: y-scope/clp#486
File: components/core/tests/test-error_handling.cpp:44-91
Timestamp: 2024-11-26T19:16:19.933Z
Learning: In `components/core/tests/test-error_handling.cpp` (C++), when specializing template methods of `ErrorCategory` via type aliases (e.g., `AlwaysSuccessErrorCategory`), it's acceptable to define these specializations in the current namespace, even though the primary template is in the `clp::error_handling` namespace, due to the use of type aliases and template instantiation.
Applied to files:
components/core/src/clp_s/timestamp_parser/ErrorCode.cpp
📚 Learning: 2024-11-26T19:10:05.051Z
Learnt from: LinZhihao-723
PR: y-scope/clp#486
File: components/core/src/clp/error_handling/ErrorCode.hpp:34-34
Timestamp: 2024-11-26T19:10:05.051Z
Learning: In the `ErrorCategory` template class in `ErrorCode.hpp`, the `name()` and `message(ErrorCodeEnum)` methods should not be declared as pure virtual functions because the class relies on template specializations, and making them pure virtual would make the class abstract, which is not intended.
Applied to files:
components/core/src/clp_s/timestamp_parser/ErrorCode.cpp
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (18)
- GitHub Check: ubuntu-jammy-dynamic-linked-bins
- GitHub Check: manylinux_2_28-x86_64-dynamic-linked-bins
- GitHub Check: centos-stream-9-static-linked-bins
- GitHub Check: ubuntu-jammy-static-linked-bins
- GitHub Check: manylinux_2_28-x86_64-static-linked-bins
- GitHub Check: musllinux_1_2-x86_64-dynamic-linked-bins
- GitHub Check: package-image
- GitHub Check: musllinux_1_2-x86_64-static-linked-bins
- GitHub Check: ubuntu-jammy-lint
- GitHub Check: centos-stream-9-dynamic-linked-bins
- GitHub Check: rust-checks (ubuntu-24.04)
- GitHub Check: rust-checks (ubuntu-22.04)
- GitHub Check: build-macos (macos-15, true)
- GitHub Check: build-macos (macos-14, false)
- GitHub Check: build-macos (macos-15, false)
- GitHub Check: build-macos (macos-14, true)
- GitHub Check: lint-check (ubuntu-24.04)
- GitHub Check: lint-check (macos-15)
🔇 Additional comments (5)
components/core/src/clp_s/CMakeLists.txt (1)
3-3: Looks good.Including the subdirectory keeps the option local, and the inner CMake handles the OFF case.
components/core/src/clp_s/timestamp_parser/ErrorCode.hpp (1)
1-21: Header is in good shape.Enum values and alias line up with the new parser errors; no gaps spotted.
components/core/src/clp_s/timestamp_parser/ErrorCode.cpp (1)
1-31: Implementation matches the spec.Error-category name and messages cover every enum path; default guard is appreciated.
components/core/CMakeLists.txt (1)
754-754: Guard the new link dependency.When
CLP_BUILD_CLP_S_TIMESTAMP_PARSERis OFF no targetclp_s::timestamp_parserexists, so this line breaks the configure step. Either add a conditional link or make the test dependency check enforce the option. The latter is discussed in options.cmake; once that’s in place this line is fine. Please wire both changes together.⛔ Skipped due to learnings
Learnt from: Bill-hbrhbr PR: y-scope/clp#1156 File: components/core/CMakeLists.txt:772-772 Timestamp: 2025-08-09T04:07:27.083Z Learning: In the CLP project's CMakeLists.txt, when reviewing changes related to the ${zstd_TARGET} variable usage in test linking, the team is planning a refactoring PR to improve this mechanism. Guards for undefined target variables should be deferred to that separate PR rather than being added in focused dependency migration PRs.components/core/src/clp_s/timestamp_parser/CMakeLists.txt (1)
1-25: Consider reshaping the header dependency.Listing
../Defs.hppas a “source” pulls it into IDE projects, but it is still owned by the timestamp_pattern target. If someone disablesCLP_BUILD_CLP_S_TIMESTAMPPATTERN, the file still exists yet exposes the parser to unwanted coupling. Prefer including it transitively via headers instead of compiling it. That aligns dependency lines with the Option validation suggested earlier.⛔ Skipped due to learnings
Learnt from: Bill-hbrhbr PR: y-scope/clp#1156 File: components/core/CMakeLists.txt:772-772 Timestamp: 2025-08-09T04:07:27.083Z Learning: In the CLP project's CMakeLists.txt, when reviewing changes related to the ${zstd_TARGET} variable usage in test linking, the team is planning a refactoring PR to improve this mechanism. Guards for undefined target variables should be deferred to that separate PR rather than being added in focused dependency migration PRs.
| option( | ||
| CLP_BUILD_CLP_S_TIMESTAMP_PARSER | ||
| "Build clp_s::timestamp_parser" | ||
| ON | ||
| ) |
There was a problem hiding this comment.
🧹 Nitpick | 🔵 Trivial
Add trailing period for option description.
All other option descriptions end in a period. Matching that pattern keeps help text tidy.
🤖 Prompt for AI Agents
In components/core/cmake/Options/options.cmake around lines 86 to 90, the option
description string "Build clp_s::timestamp_parser" is missing a trailing period;
update the description to end with a period so it matches the punctuation style
of other options (e.g., "Build clp_s::timestamp_parser.").
| src/clp_s/TimestampDictionaryWriter.hpp | ||
| src/clp_s/TimestampEntry.cpp | ||
| src/clp_s/TimestampEntry.hpp | ||
| src/clp_s/timestamp_parser/test/test_TimestampParser.cpp |
There was a problem hiding this comment.
Protect test sources behind the build option.
If CLP_BUILD_CLP_S_TIMESTAMP_PARSER is OFF, this source file should not be compiled. Consider wrapping the addition with if(CLP_BUILD_CLP_S_TIMESTAMP_PARSER) to avoid missing-file errors when the feature is disabled.
set(SOURCE_FILES_clp_s_unitTest
...
- src/clp_s/timestamp_parser/test/test_TimestampParser.cpp
+ $<$<BOOL:${CLP_BUILD_CLP_S_TIMESTAMP_PARSER}>:src/clp_s/timestamp_parser/test/test_TimestampParser.cpp>
...
)📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| src/clp_s/timestamp_parser/test/test_TimestampParser.cpp | |
| set(SOURCE_FILES_clp_s_unitTest | |
| ... | |
| $<$<BOOL:${CLP_BUILD_CLP_S_TIMESTAMP_PARSER}>:src/clp_s/timestamp_parser/test/test_TimestampParser.cpp> | |
| ... | |
| ) |
🤖 Prompt for AI Agents
In components/core/CMakeLists.txt around line 409, the test source
src/clp_s/timestamp_parser/test/test_TimestampParser.cpp is unconditionally
listed which causes missing-file errors when CLP_BUILD_CLP_S_TIMESTAMP_PARSER is
OFF; wrap the addition of this source (or the whole target that references it)
in an if(CLP_BUILD_CLP_S_TIMESTAMP_PARSER) ... endif() block so the file is only
appended/added when the feature is enabled, or alternatively conditionally call
target_sources/add_executable only when that CMake option is TRUE.
components/core/src/clp_s/timestamp_parser/test/test_TimestampParser.cpp
Show resolved
Hide resolved
| * We support the following format specifiers: | ||
| * | ||
| * - \y Zero-padded year in century (69-99 -> 1969-1999, 00-68 -> 2000-2068). | ||
| * - \Y Zero-padded year (0000-9999). | ||
| * - \B Full month name (e.g., January). | ||
| * - \b Abbreviated month name (e.g., Jan). | ||
| * - \m Zero-padded month (01-12). | ||
| * - \d Zero-padded day in month (01-31). | ||
| * - \e Space-padded day in month( 1-31). | ||
| * - \a Abbreviated day in week (e.g., Mon). | ||
| * - \p Part of day (AM/PM). | ||
| * - \H 24-hour clock, zero-padded hour (00-23). | ||
| * - \k 24-hour clock, space-padded hour ( 0-23). | ||
| * - \I 12-hour clock, zero-padded hour (01-12). | ||
| * - \l 12-hour clock, space-padded hour ( 1-12). | ||
| * - \M Zero-padded minute (00-59). | ||
| * - \S Zero-padded second (00-60) (60 for leap seconds). | ||
| * - \3 Zero-padded millisecond (000-999). | ||
| * - \6 Zero-padded microsecond (000000-999999). | ||
| * - \9 Zero-padded nanosecond (000000000-999999999). | ||
| * - \T Zero-padded fractional second, up to nanoseconds, without trailing zeroes. | ||
| * - \E Epoch seconds. | ||
| * - \L Epoch miLliseconds. | ||
| * - \C Epoch miCroseconds. | ||
| * - \N Epoch Nanoseconds. | ||
| * - \z{...} Specific timezone, described by content between {}. | ||
| * - \\ Literal backslash. | ||
| * | ||
| * We also support the following CAT sequences: | ||
| * | ||
| * - \Z Generic timezone -- resolves to literal content, and potentially \z{...}. | ||
| * - \? Generic fractional second -- resolves to \3, \6, \9, or \T. | ||
| * - \P Unknown-precision epoch time -- resolves to \E, \L, \C, or \N based on heuristic. | ||
| * |
There was a problem hiding this comment.
Align the documented specifier list with the implemented behaviour.
The comment advertises support for \p, \H, \k, \I, \l, \M, \S, \3, \6, \9, \T, \E, \L, \C, \N, \z{…}, \Z, \?, and \P, but the implementation presently returns FormatSpecifierNotImplemented for each of them. Either trim the list to what is truly supported in this PR or implement the missing cases before shipping; otherwise, callers relying on the documented contract will hit runtime failures.
🤖 Prompt for AI Agents
components/core/src/clp_s/timestamp_parser/TimestampParser.hpp lines 31-64: the
comment block lists many specifiers that are not implemented and currently cause
FormatSpecifierNotImplemented at runtime; update the documentation to match
implemented behavior by removing or marking as unsupported the specifiers that
return FormatSpecifierNotImplemented (specifically \p, \H, \k, \I, \l, \M, \S,
\3, \6, \9, \T, \E, \L, \C, \N, \z{...}, \Z, \?, \P), or alternatively implement
those cases in the parser before shipping; pick one approach — for this PR, trim
the comment to only the specifiers the code currently handles, add a short TODO
note listing the unimplemented specifiers and reference an issue/PR for future
work, and run/update unit tests to ensure the doc/test matrix matches behavior.
There was a problem hiding this comment.
Actionable comments posted: 3
📜 Review details
Configuration used: CodeRabbit UI
Review profile: ASSERTIVE
Plan: Pro
📒 Files selected for processing (1)
components/core/src/clp_s/timestamp_parser/test/test_TimestampParser.cpp(1 hunks)
🧰 Additional context used
📓 Path-based instructions (1)
**/*.{cpp,hpp,java,js,jsx,tpp,ts,tsx}
⚙️ CodeRabbit configuration file
- Prefer
false == <expression>rather than!<expression>.
Files:
components/core/src/clp_s/timestamp_parser/test/test_TimestampParser.cpp
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (18)
- GitHub Check: build-macos (macos-15, false)
- GitHub Check: build-macos (macos-15, true)
- GitHub Check: build-macos (macos-14, true)
- GitHub Check: ubuntu-jammy-lint
- GitHub Check: musllinux_1_2-x86_64-static-linked-bins
- GitHub Check: musllinux_1_2-x86_64-dynamic-linked-bins
- GitHub Check: ubuntu-jammy-static-linked-bins
- GitHub Check: package-image
- GitHub Check: ubuntu-jammy-dynamic-linked-bins
- GitHub Check: centos-stream-9-static-linked-bins
- GitHub Check: centos-stream-9-dynamic-linked-bins
- GitHub Check: manylinux_2_28-x86_64-dynamic-linked-bins
- GitHub Check: manylinux_2_28-x86_64-static-linked-bins
- GitHub Check: lint-check (macos-15)
- GitHub Check: lint-check (ubuntu-24.04)
- GitHub Check: rust-checks (macos-15)
- GitHub Check: rust-checks (ubuntu-22.04)
- GitHub Check: rust-checks (ubuntu-24.04)
🔇 Additional comments (4)
components/core/src/clp_s/timestamp_parser/test/test_TimestampParser.cpp (4)
13-31: LGTM: Helper function declarations are clear.The documentation and signatures are appropriate for the test helpers.
33-45: LGTM: Specifier validation logic is sound.The trailing literal technique ensures exact consumption, and the implementation correctly follows the
false == <expression>coding guideline at line 43.
59-107: The past review comment about inclusive bounds is based on a misunderstanding.The implementation at line 52 uses
i <= end, which produces a closed range[begin, end]. Therefore:
- Line 99:
generate_padded_numbers_in_range(1, 12, ...)does generate months01through12(inclusive).- Lines 102 & 105:
generate_padded_numbers_in_range(1, 31, ...)does generate days01through31(inclusive).The confusion arose because the function name suggests half-open
[begin, end)semantics, but the implementation uses closed[begin, end]semantics. The current test coverage is correct.
108-124: LGTM: Day-of-week validation logic is correct.The timestamps align with the Unix epoch (Thursday, January 1, 1970), and the pattern
"\\d \\aa"correctly validates day-of-week alignment. Line 123 properly follows thefalse == <expression>coding guideline.
components/core/src/clp_s/timestamp_parser/test/test_TimestampParser.cpp
Outdated
Show resolved
Hide resolved
components/core/src/clp_s/timestamp_parser/test/test_TimestampParser.cpp
Show resolved
Hide resolved
| TEST_CASE("timestamp_parser_parse_timestamp", "[clp-s][timestamp-parser]") { | ||
| SECTION("Format specifiers accept valid content.") { | ||
| auto const two_digit_years{generate_padded_numbers_in_range(0, 99, 2, '0')}; | ||
| assert_specifier_accepts_valid_content('y', two_digit_years); | ||
|
|
||
| auto const four_digit_years{generate_padded_numbers_in_range(0, 9999, 4, '0')}; | ||
| assert_specifier_accepts_valid_content('Y', four_digit_years); | ||
|
|
||
| std::vector<std::string> const months{ | ||
| "January", | ||
| "February", | ||
| "March", | ||
| "April", | ||
| "May", | ||
| "June", | ||
| "July", | ||
| "August", | ||
| "September", | ||
| "October", | ||
| "November", | ||
| "December" | ||
| }; | ||
| assert_specifier_accepts_valid_content('B', months); | ||
|
|
||
| std::vector<std::string> const abbreviated_months{ | ||
| "Jan", | ||
| "Feb", | ||
| "Mar", | ||
| "Apr", | ||
| "May", | ||
| "Jun", | ||
| "Jul", | ||
| "Aug", | ||
| "Sep", | ||
| "Oct", | ||
| "Nov", | ||
| "Dec" | ||
| }; | ||
| assert_specifier_accepts_valid_content('b', abbreviated_months); | ||
|
|
||
| auto const two_digit_months{generate_padded_numbers_in_range(1, 12, 2, '0')}; | ||
| assert_specifier_accepts_valid_content('m', two_digit_months); | ||
|
|
||
| auto const two_digit_days{generate_padded_numbers_in_range(1, 31, 2, '0')}; | ||
| assert_specifier_accepts_valid_content('d', two_digit_days); | ||
|
|
||
| auto const space_padded_days(generate_padded_numbers_in_range(1, 31, 2, ' ')); | ||
| assert_specifier_accepts_valid_content('e', space_padded_days); | ||
|
|
||
| // The parser asserts that the day of the week in the timestamp is actually correct, so we | ||
| // need to include some extra date information to have days of the week line up. | ||
| std::vector<std::string> abbreviated_day_in_week_timestamps{ | ||
| "04 Sun", | ||
| "05 Mon", | ||
| "06 Tue", | ||
| "07 Wed", | ||
| "01 Thu", // Thursday January 1st, 1970 | ||
| "02 Fri", | ||
| "03 Sat" | ||
| }; | ||
| for (auto const& day_in_week_timestamp : abbreviated_day_in_week_timestamps) { | ||
| std::string generated_pattern; | ||
| auto const timestamp{fmt::format("{}a", day_in_week_timestamp)}; | ||
| auto const result{parse_timestamp(timestamp, "\\d \\aa", generated_pattern)}; | ||
| REQUIRE(false == result.has_error()); | ||
| } | ||
| } | ||
| } |
There was a problem hiding this comment.
🧹 Nitpick | 🔵 Trivial
Consider adding negative test cases.
The current test suite validates successful parsing of valid inputs. To improve robustness, consider adding test cases that verify expected failures (e.g., invalid month numbers, malformed patterns, mismatched day-of-week).
Example:
SECTION("Format specifiers reject invalid content.") {
std::string generated_pattern;
auto result = parse_timestamp("13a", "\\ma", generated_pattern); // Invalid month
REQUIRE(result.has_error());
result = parse_timestamp("32a", "\\da", generated_pattern); // Invalid day
REQUIRE(result.has_error());
}🤖 Prompt for AI Agents
components/core/src/clp_s/timestamp_parser/test/test_TimestampParser.cpp lines
59-126: add negative test cases that assert parsing fails for invalid inputs;
create a new SECTION("Format specifiers reject invalid content.") and call
parse_timestamp with examples like an out-of-range month ("13a" with pattern
"\ma"), out-of-range day ("32a" with pattern "\da"), malformed patterns, and a
timestamp whose day-of-week doesn't match, then REQUIRE that result.has_error()
is true for each case to ensure the parser correctly reports errors.
| #ifndef CLP_S_TIMESTAMPPARSER_HPP | ||
| #define CLP_S_TIMESTAMPPARSER_HPP |
There was a problem hiding this comment.
| #ifndef CLP_S_TIMESTAMPPARSER_HPP | |
| #define CLP_S_TIMESTAMPPARSER_HPP | |
| #ifndef CLP_S_TIMESTAMP_PARSER_TIMESTAMPPARSER_HPP | |
| #define CLP_S_TIMESTAMP_PARSER_TIMESTAMPPARSER_HPP |
Also need to update the comment in #endif
| #ifndef CLP_S_TIMESTAMPPARSERERRORCODE_HPP | ||
| #define CLP_S_TIMESTAMPPARSERERRORCODE_HPP |
There was a problem hiding this comment.
Same, this seems out dated.
| * @param pattern A timestamp pattern made up of literals, format specifiers, and potentially CAT | ||
| * sequences. | ||
| * @param generated_pattern A buffer where a newly-generated timestamp pattern can be written, if | ||
| * necessary. | ||
| * @return On success: | ||
| * - The timestamp in epoch nanoseconds. | ||
| * - A string_view of the timestamp pattern that corresponds to the timestamp. | ||
| * - Lifetime is least of `pattern` and `generated_pattern`. | ||
| * On error: | ||
| * - A `clp_s::timestamp_parser::ErrorCode`. |
There was a problem hiding this comment.
Haven't reviewed the content yet. But:
- We don't need to indent when wrapping to a new line (unless we recently udpated the guideline that I wasn't aware of)
- We have a guideline to document result return type. Here's an example that also returns a pair:
In general the rule for result return is:
A result containing xxxx on success, or an error code indicating the failure:\n List all possible error codes, or useA void result containing xxx on successif the return type isvoid.
Can u go through other docstrings to ensure these are all applied?
| * @param value The integer value, returned by reference. | ||
| * @return Whether conversion was successful. | ||
| */ | ||
| [[nodiscard]] auto | ||
| convert_padded_string_to_number(std::string_view str, char padding_character, int& value) -> bool; |
There was a problem hiding this comment.
We should modernize this by returning std::optional<int>.
There was a problem hiding this comment.
Actionable comments posted: 1
♻️ Duplicate comments (1)
components/core/src/clp_s/timestamp_parser/TimestampParser.hpp (1)
32-65: Document only the implemented specifiers or mark unimplemented ones clearly.The documentation lists many specifiers (
\p,\H,\k,\I,\l,\M,\S,\3,\6,\9,\T,\E,\L,\C,\N,\z{...},\Z,\?,\P) that returnFormatSpecifierNotImplementedin the implementation. This creates a mismatch between the documented API surface and actual behaviour, which can mislead users.Option 1 (preferred for this PR): Remove the unimplemented specifiers from this list and add a note:
* We support the following format specifiers: * * - \y Zero-padded year in century (69-99 -> 1969-1999, 00-68 -> 2000-2068). * - \Y Zero-padded year (0000-9999). * - \B Full month name (e.g., January). * - \b Abbreviated month name (e.g., Jan). * - \m Zero-padded month (01-12). * - \d Zero-padded day in month (01-31). * - \e Space-padded day in month( 1-31). * - \a Abbreviated day in week (e.g., Mon). - * - \p Part of day (AM/PM). - * - \H 24-hour clock, zero-padded hour (00-23). - * - \k 24-hour clock, space-padded hour ( 0-23). - * - \I 12-hour clock, zero-padded hour (01-12). - * - \l 12-hour clock, space-padded hour ( 1-12). - * - \M Zero-padded minute (00-59). - * - \S Zero-padded second (00-60) (60 for leap seconds). - * - \3 Zero-padded millisecond (000-999). - * - \6 Zero-padded microsecond (000000-999999). - * - \9 Zero-padded nanosecond (000000000-999999999). - * - \T Zero-padded fractional second, up to nanoseconds, without trailing zeroes. - * - \E Epoch seconds. - * - \L Epoch miLliseconds. - * - \C Epoch miCroseconds. - * - \N Epoch Nanoseconds. - * - \z{...} Specific timezone, described by content between {}. * - \\ Literal backslash. + * + * Additional format specifiers (\p, \H, \k, \I, \l, \M, \S, \3, \6, \9, \T, \E, \L, \C, \N, \z{...}) + * and CAT sequences (\Z, \?, \P) are planned for future implementation. * - * We also support the following CAT sequences: - * - * - \Z Generic timezone -- resolves to literal content, and potentially \z{...}. - * - \? Generic fractional second -- resolves to \3, \6, \9, or \T. - * - \P Unknown-precision epoch time -- resolves to \E, \L, \C, or \N based on a heuristic. - *Option 2 (if unimplemented specifiers must remain): Clearly mark them as unimplemented:
* We support the following format specifiers: * * - \y Zero-padded year in century (69-99 -> 1969-1999, 00-68 -> 2000-2068). * ... * - \a Abbreviated day in week (e.g., Mon). + * + * The following format specifiers are not yet implemented and will return `FormatSpecifierNotImplemented`: + * * - \p Part of day (AM/PM). * ...
📜 Review details
Configuration used: CodeRabbit UI
Review profile: ASSERTIVE
Plan: Pro
📒 Files selected for processing (4)
components/core/src/clp_s/timestamp_parser/ErrorCode.hpp(1 hunks)components/core/src/clp_s/timestamp_parser/TimestampParser.cpp(1 hunks)components/core/src/clp_s/timestamp_parser/TimestampParser.hpp(1 hunks)components/core/src/clp_s/timestamp_parser/test/test_TimestampParser.cpp(1 hunks)
🧰 Additional context used
📓 Path-based instructions (1)
**/*.{cpp,hpp,java,js,jsx,tpp,ts,tsx}
⚙️ CodeRabbit configuration file
- Prefer
false == <expression>rather than!<expression>.
Files:
components/core/src/clp_s/timestamp_parser/test/test_TimestampParser.cppcomponents/core/src/clp_s/timestamp_parser/TimestampParser.hppcomponents/core/src/clp_s/timestamp_parser/TimestampParser.cppcomponents/core/src/clp_s/timestamp_parser/ErrorCode.hpp
🧠 Learnings (1)
📚 Learning: 2024-11-26T19:12:09.102Z
Learnt from: LinZhihao-723
PR: y-scope/clp#486
File: components/core/src/clp/error_handling/ErrorCode.hpp:131-134
Timestamp: 2024-11-26T19:12:09.102Z
Learning: In the error handling system (`components/core/src/clp/error_handling/ErrorCode.hpp`), the `ErrorCode` class is intentionally designed to wrap the `ErrorCodeEnum`, not to use the enum directly. This wrapper provides a general interface for adding customized error enums.
Applied to files:
components/core/src/clp_s/timestamp_parser/ErrorCode.hpp
🔇 Additional comments (28)
components/core/src/clp_s/timestamp_parser/test/test_TimestampParser.cpp (8)
1-9: LGTM! Clean includes and namespace setup.The includes are properly organized and the namespace structure is appropriate for the test file.
12-30: LGTM! Helper function declarations are well-documented.The docstrings clearly describe the purpose and parameters of the helper functions.
32-44: LGTM! Test helper follows coding guidelines.The function correctly uses
false == result.has_error()as per the coding guidelines.
46-55: LGTM! Range generation function is correctly implemented.The function now properly supports inclusive ranges with the updated precondition
begin <= endand the loop conditioni <= end, addressing the previous review feedback.
58-65: LGTM! Year format specifiers are thoroughly tested.The test cases appropriately cover both 2-digit and 4-digit year formats with comprehensive ranges.
66-96: LGTM! Month format specifiers are well-tested.The test cases cover full month names, abbreviated names, and numeric formats comprehensively.
107-124: LGTM! Day-of-week validation is properly tested.The test correctly validates that the parser checks day-of-week alignment with actual dates, using Thursday January 1st, 1970 as the reference point. The test also follows the coding guideline by using
false == result.has_error().
98-106: Inclusive upper bounds are correctly tested
generate_padded_numbers_in_rangeusesi <= end, and test ranges for months (1–12) and days (1–31) include the upper bounds.components/core/src/clp_s/timestamp_parser/ErrorCode.hpp (1)
1-21: LGTM! Error code definitions follow the established pattern.The error code enum and type alias follow the ystdlib error handling pattern consistently. The enum values are descriptive and cover the expected error scenarios for timestamp parsing. Based on learnings, the wrapper pattern using
ErrorCode = ystdlib::error_handling::ErrorCode<ErrorCodeEnum>is the intentional design.components/core/src/clp_s/timestamp_parser/TimestampParser.hpp (4)
1-12: LGTM! Standard header structure.The include guard, includes, and namespace declaration are properly structured.
14-31: LGTM! The introduction and high-level documentation are clear.The documentation effectively explains the purpose of the parser, the concept of format specifiers vs CAT sequences, and the motivation for CAT sequences.
66-84: LGTM! The parameter and return documentation is comprehensive.The documentation clearly describes the function parameters, return value structure, and all possible error codes.
85-92: LGTM! Function signature is well-defined.The use of
[[nodiscard]], proper parameter types, and Result return type align with modern C++ best practices.components/core/src/clp_s/timestamp_parser/TimestampParser.cpp (15)
1-17: LGTM! Includes are appropriate.The standard library headers, third-party dependencies (date, string_utils, ystdlib), and local headers are all properly included.
19-72: LGTM! Constants are well-defined.The parsing constraints and string literal arrays for days/months are clearly defined with appropriate naming and scope.
73-93: LGTM! Helper function declarations are properly documented.The docstrings clearly describe the template parameter, parameters, and return values.
112-123: LGTM! Prefix matching helper is correctly implemented.The function properly iterates through candidates and returns the matching index or an error code.
125-156: LGTM! Parser initialization and escape handling are correct.The function properly initializes state variables and handles escape sequences. The use of
false == escapedfollows the coding guidelines.
158-185: LGTM! Two-digit year parsing with century inference is correct.The implementation properly handles the 69-99 → 1969-1999 and 00-68 → 2000-2068 mapping as documented. The bounds checking and error handling are appropriate.
186-208: LGTM! Four-digit year parsing is correct.The implementation validates the year range and properly handles padding.
209-227: LGTM! Month name parsing is correct.Both full and abbreviated month name parsing use the helper function appropriately and convert the zero-based index to a one-based month value.
228-250: LGTM! Numeric month parsing is correct.The implementation properly validates the month range (1-12) with appropriate error handling.
251-296: LGTM! Day parsing handles both padding types correctly.The implementation properly handles both zero-padded (
\d) and space-padded (\e) days with appropriate validation.
297-306: LGTM! Day-of-week parsing is correct.The implementation stores the parsed day-of-week index for later validation against the computed date.
307-336: LGTM! Unimplemented specifiers and escape handling are correct.The implementation appropriately returns
FormatSpecifierNotImplementedfor specifiers not yet implemented and properly handles literal backslashes.
339-352: LGTM! Final validation checks are appropriate.The code correctly validates that both pattern and timestamp are fully consumed and prevents mixing date-based and epoch-number representations.
354-372: LGTM! Date validation and day-of-week verification are correct.The implementation uses the date library to validate the date and correctly verifies day-of-week alignment when provided. The arithmetic for computing the actual day-of-week index is correct (Sunday = 0).
374-379: LGTM! Epoch time conversion is correct.The implementation properly constructs a time_point and converts it to epoch nanoseconds using chrono duration_cast.
|
|
||
| // Leave at least one character for parsing to ensure we actually parse number content. | ||
| size_t i{}; | ||
| for (; i < (str.size() - 1) && padding_character == str[i]; ++i) {} |
There was a problem hiding this comment.
Nit: I understand we've done in-bound check already, but shall we use .at(i) to access the char to resolve the clang-tidy warning (might not show up with the current clang-tidy version)?
| * @return A result containing the index of the matching prefix in the candidates array, or | ||
| * `clp_s::timestamp_parser::ErrorCodeEnum::IncompatibleTimestampPattern` on failure. | ||
| */ | ||
| template <typename CandidateArrayType> |
There was a problem hiding this comment.
Do we need a template parameter?
- If to use a template, how about using range?
- If not, we can use a span.
There was a problem hiding this comment.
I think we should be able to make that switch, yeah. Will try using range.
There was a problem hiding this comment.
Actually, I'll probably go with span.
| int parsed_year{cDefaultYear}; | ||
| int parsed_month{cDefaultMonth}; | ||
| int parsed_day{cDefaultDay}; | ||
| std::optional<int> day_of_week_idx; |
There was a problem hiding this comment.
optional_day_of_week_idx?
There was a problem hiding this comment.
Can u also address clang-tidy violations in this test file?
There was a problem hiding this comment.
Actionable comments posted: 2
♻️ Duplicate comments (1)
components/core/src/clp_s/timestamp_parser/TimestampParser.hpp (1)
32-65: Document only what is actually implemented in this PR.The list marks many specifiers as “We support …” (e.g., \p, \H, \k, \I, \l, \M, \S, \3, \6, \9, \T, \E, \L, \C, \N, \z{…}, \Z, ?, \P), but the implementation currently returns FormatSpecifierNotImplemented for all of them. This will mislead callers.
Please either:
- Trim this section to the specifiers implemented now (y, Y, B, b, m, d, e, a, and literal backslash), and move the rest to a “Planned (not yet implemented)” list with a TODO/issue reference; or
- Implement the missing cases before merging.
Suggested doc-only diff (illustrative):
- * We support the following format specifiers: + * Supported in this PR: * - \y ... * - \Y ... * - \B ... * - \b ... * - \m ... * - \d ... * - \e ... * - \a ... * - \\ Literal backslash. - * - * We also support the following CAT sequences: - * - \Z ... - * - \? ... - * - \P ... + * + * Planned (not yet implemented in this PR): + * - \p, \H, \k, \I, \l, \M, \S, \3, \6, \9, \T, \E, \L, \C, \N, \z{...}, \Z, \?, \P. + * Returns ErrorCodeEnum::FormatSpecifierNotImplemented for now. TODO: implement.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: ASSERTIVE
Plan: Pro
📒 Files selected for processing (2)
components/core/src/clp_s/timestamp_parser/TimestampParser.cpp(1 hunks)components/core/src/clp_s/timestamp_parser/TimestampParser.hpp(1 hunks)
🧰 Additional context used
📓 Path-based instructions (1)
**/*.{cpp,hpp,java,js,jsx,tpp,ts,tsx}
⚙️ CodeRabbit configuration file
- Prefer
false == <expression>rather than!<expression>.
Files:
components/core/src/clp_s/timestamp_parser/TimestampParser.hppcomponents/core/src/clp_s/timestamp_parser/TimestampParser.cpp
🧠 Learnings (2)
📚 Learning: 2024-11-26T19:12:09.102Z
Learnt from: LinZhihao-723
PR: y-scope/clp#486
File: components/core/src/clp/error_handling/ErrorCode.hpp:131-134
Timestamp: 2024-11-26T19:12:09.102Z
Learning: In the error handling system (`components/core/src/clp/error_handling/ErrorCode.hpp`), the `ErrorCode` class is intentionally designed to wrap the `ErrorCodeEnum`, not to use the enum directly. This wrapper provides a general interface for adding customized error enums.
Applied to files:
components/core/src/clp_s/timestamp_parser/TimestampParser.hpp
📚 Learning: 2025-01-29T22:16:52.665Z
Learnt from: haiqi96
PR: y-scope/clp#700
File: components/core/src/clp/streaming_archive/ArchiveMetadata.cpp:29-46
Timestamp: 2025-01-29T22:16:52.665Z
Learning: In C++ code, when throwing exceptions, prefer throwing the original return code (RC) from lower-level operations instead of a generic error code to preserve error context and aid in debugging.
Applied to files:
components/core/src/clp_s/timestamp_parser/TimestampParser.hpp
🔇 Additional comments (2)
components/core/src/clp_s/timestamp_parser/TimestampParser.hpp (1)
86-90: Return type and error category look correct.Using Result<..., ErrorCode> aligns with the project’s error-handling approach of wrapping custom enums in an ErrorCode wrapper. No change requested. Based on learnings.
components/core/src/clp_s/timestamp_parser/TimestampParser.cpp (1)
309-329: Unimplemented specifiers return FormatSpecifierNotImplemented (OK for phased rollout).The switch correctly returns FormatSpecifierNotImplemented for not-yet-implemented cases. Ensure the header doc is updated accordingly so callers know these are not available in this PR.
There was a problem hiding this comment.
Actionable comments posted: 1
♻️ Duplicate comments (5)
components/core/cmake/Options/options.cmake (2)
161-170: Tests should validate parser option is enabled.unitTest links clp_s::timestamp_parser; enforce CLP_BUILD_CLP_S_TIMESTAMP_PARSER in test deps to avoid configure-time failures.
function(validate_clp_tests_dependencies) validate_clp_dependencies_for_target(CLP_BUILD_TESTING CLP_BUILD_CLP_REGEX_UTILS CLP_BUILD_CLP_STRING_UTILS CLP_BUILD_CLP_S_SEARCH_AST CLP_BUILD_CLP_S_SEARCH_KQL CLP_BUILD_CLP_S_SEARCH_SQL CLP_BUILD_CLP_S_TIMESTAMPPATTERN + CLP_BUILD_CLP_S_TIMESTAMP_PARSER ) endfunction()
86-90: Add trailing period to option description.option( CLP_BUILD_CLP_S_TIMESTAMP_PARSER - "Build clp_s::timestamp_parser" + "Build clp_s::timestamp_parser." ON )components/core/src/clp_s/timestamp_parser/TimestampParser.hpp (1)
31-64: Align documented specifiers with implemented behaviour (or mark as TODO).The header advertises specifiers that currently return FormatSpecifierNotImplemented (p, H, k, I, l, M, S, 3, 6, 9, T, E, L, C, N, z{…}, Z, ?, P). Either trim the list to y, Y, B, b, m, d, e, a, and '\' (implemented in this PR) or clearly mark the rest as “not yet implemented” with a TODO and link to the follow‑up PR/issue.
- * We support the following format specifiers: + * Supported in this PR: * - * - \y ... + * - \y ... * - \Y ... * - \B ... * - \b ... * - \m ... * - \d ... * - \e ... * - \a ... * - \\ Literal backslash. * - * We also support the following CAT sequences: - * ... + * Planned (not yet implemented in this PR; will return FormatSpecifierNotImplemented): + * \p, \H, \k, \I, \l, \M, \S, \3, \6, \9, \T, \E, \L, \C, \N, \z{…}, \Z, \?, \P. + * TODO: implement in follow‑up PR.components/core/src/clp_s/timestamp_parser/test/test_TimestampParser.cpp (1)
61-128: Add negative tests for invalid inputs.Strengthen the suite with failure cases (out-of-range month/day, malformed patterns, DOW mismatch, unknown specifiers).
SECTION("Format specifiers reject invalid content.") { std::string gen; REQUIRE(true == parse_timestamp("13a", "\\ma", gen).has_error()); // invalid month REQUIRE(true == parse_timestamp("32a", "\\da", gen).has_error()); // invalid day REQUIRE(true == parse_timestamp("04 Mon", "\\d \\aa", gen).has_error()); // DOW mismatch REQUIRE(true == parse_timestamp("", "\\", gen).has_error()); // orphaned escape }components/core/src/clp_s/timestamp_parser/TimestampParser.cpp (1)
140-143: Dead code / forward-compatibility for epoch paths.number_type_representation is const false, so mixing/epoch branches cannot execute. Make it non-const and add a short TODO for the epoch specifiers to avoid future churn.
- bool const number_type_representation{false}; + // TODO(PR: follow-up): Set when epoch-number specifiers (\E, \L, \C, \N, \P) are parsed. + bool number_type_representation{false};
📜 Review details
Configuration used: CodeRabbit UI
Review profile: ASSERTIVE
Plan: Pro
📒 Files selected for processing (6)
components/core/cmake/Options/options.cmake(3 hunks)components/core/src/clp_s/CMakeLists.txt(1 hunks)components/core/src/clp_s/timestamp_parser/CMakeLists.txt(1 hunks)components/core/src/clp_s/timestamp_parser/TimestampParser.cpp(1 hunks)components/core/src/clp_s/timestamp_parser/TimestampParser.hpp(1 hunks)components/core/src/clp_s/timestamp_parser/test/test_TimestampParser.cpp(1 hunks)
🧰 Additional context used
📓 Path-based instructions (1)
**/*.{cpp,hpp,java,js,jsx,tpp,ts,tsx}
⚙️ CodeRabbit configuration file
- Prefer
false == <expression>rather than!<expression>.
Files:
components/core/src/clp_s/timestamp_parser/TimestampParser.hppcomponents/core/src/clp_s/timestamp_parser/test/test_TimestampParser.cppcomponents/core/src/clp_s/timestamp_parser/TimestampParser.cpp
🧠 Learnings (3)
📚 Learning: 2024-11-26T19:12:09.102Z
Learnt from: LinZhihao-723
PR: y-scope/clp#486
File: components/core/src/clp/error_handling/ErrorCode.hpp:131-134
Timestamp: 2024-11-26T19:12:09.102Z
Learning: In the error handling system (`components/core/src/clp/error_handling/ErrorCode.hpp`), the `ErrorCode` class is intentionally designed to wrap the `ErrorCodeEnum`, not to use the enum directly. This wrapper provides a general interface for adding customized error enums.
Applied to files:
components/core/src/clp_s/timestamp_parser/TimestampParser.hpp
📚 Learning: 2025-01-29T22:16:52.665Z
Learnt from: haiqi96
PR: y-scope/clp#700
File: components/core/src/clp/streaming_archive/ArchiveMetadata.cpp:29-46
Timestamp: 2025-01-29T22:16:52.665Z
Learning: In C++ code, when throwing exceptions, prefer throwing the original return code (RC) from lower-level operations instead of a generic error code to preserve error context and aid in debugging.
Applied to files:
components/core/src/clp_s/timestamp_parser/TimestampParser.hpp
📚 Learning: 2025-08-09T04:07:27.083Z
Learnt from: Bill-hbrhbr
PR: y-scope/clp#1156
File: components/core/CMakeLists.txt:772-772
Timestamp: 2025-08-09T04:07:27.083Z
Learning: In the CLP project's CMakeLists.txt, when reviewing changes related to the ${zstd_TARGET} variable usage in test linking, the team is planning a refactoring PR to improve this mechanism. Guards for undefined target variables should be deferred to that separate PR rather than being added in focused dependency migration PRs.
Applied to files:
components/core/cmake/Options/options.cmake
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (12)
- GitHub Check: musllinux_1_2-x86_64-dynamic-linked-bins
- GitHub Check: package-image
- GitHub Check: musllinux_1_2-x86_64-static-linked-bins
- GitHub Check: ubuntu-jammy-dynamic-linked-bins
- GitHub Check: ubuntu-jammy-static-linked-bins
- GitHub Check: ubuntu-jammy-lint
- GitHub Check: manylinux_2_28-x86_64-dynamic-linked-bins
- GitHub Check: manylinux_2_28-x86_64-static-linked-bins
- GitHub Check: centos-stream-9-dynamic-linked-bins
- GitHub Check: centos-stream-9-static-linked-bins
- GitHub Check: lint-check (macos-15)
- GitHub Check: build (macos-15)
🔇 Additional comments (2)
components/core/src/clp_s/CMakeLists.txt (1)
4-4: Build wiring looks good.Adding the subdirectory here is appropriate; target creation is already gated inside its CMake. No further action.
components/core/src/clp_s/timestamp_parser/CMakeLists.txt (1)
17-24: Confirm include scope is intentional.PUBLIC include dir resolves to components/core/src, enabling includes like clp_s/timestamp_parser/TimestampParser.hpp. If you intended to expose only clp_s, this is fine; otherwise narrow the path.
| // Do not allow trailing unmatched content. | ||
| if (pattern_idx != pattern.size() || timestamp_idx != timestamp.size()) { | ||
| { | ||
| return ErrorCode{ErrorCodeEnum::IncompatibleTimestampPattern}; | ||
| } | ||
| } |
There was a problem hiding this comment.
Bug: Trailing orphaned escape is accepted as a valid pattern.
If pattern ends with a single backslash and timestamp is empty (or fully consumed), the loop exits with escaped==true and the function returns a valid epoch for defaults. This should be InvalidTimestampPattern.
- // Do not allow trailing unmatched content.
+ // Orphaned escape at end of pattern is invalid.
+ if (true == escaped) {
+ return ErrorCode{ErrorCodeEnum::InvalidTimestampPattern};
+ }
+
+ // Do not allow trailing unmatched content.
if (pattern_idx != pattern.size() || timestamp_idx != timestamp.size()) {
{
return ErrorCode{ErrorCodeEnum::IncompatibleTimestampPattern};
}
}📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| // Do not allow trailing unmatched content. | |
| if (pattern_idx != pattern.size() || timestamp_idx != timestamp.size()) { | |
| { | |
| return ErrorCode{ErrorCodeEnum::IncompatibleTimestampPattern}; | |
| } | |
| } | |
| // Orphaned escape at end of pattern is invalid. | |
| if (true == escaped) { | |
| return ErrorCode{ErrorCodeEnum::InvalidTimestampPattern}; | |
| } | |
| // Do not allow trailing unmatched content. | |
| if (pattern_idx != pattern.size() || timestamp_idx != timestamp.size()) { | |
| { | |
| return ErrorCode{ErrorCodeEnum::IncompatibleTimestampPattern}; | |
| } | |
| } |
🤖 Prompt for AI Agents
In components/core/src/clp_s/timestamp_parser/TimestampParser.cpp around lines
340 to 345, the function currently accepts a pattern that ends with a single
trailing backslash (escaped == true) as valid; detect when the parser finishes
with escaped == true or when the pattern has an orphaned escape and return
ErrorCode{ErrorCodeEnum::InvalidTimestampPattern} instead of treating it as
valid. Add a check after the parsing loop to test the escaped flag (or
equivalently ensure the pattern was not terminated while in escape mode) and
return the InvalidTimestampPattern error before returning success.
2 participants
Description
This PR implements part of the parsing side of a new timestamp utility that will allow us to consistently parse timestamps into a known precision, support timezones, and ease maintenance of timestamp patterns.
This initial PR includes all of the necessary boilerplate for the parsing side (docstring for the parsing function, error codes + error category), implements about half of the planned format specifiers, and adds basic tests to ensure that all of the format specifiers that have been implemented so far can correctly parse the kind of content that they're supposed to accept.
Tests that ensure that whole timestamps are parsed correctly will be added in a future PR.
For more context on the project, the full design doc is available upon request.
Checklist
breaking change.
Validation performed
Summary by CodeRabbit
New Features
Configuration
Errors
Tests