Skip to content

feat(clp-s::timestamp_parser): Add support for ignoring quotes in quoted timestamp patterns.#1847

Merged
gibber9809 merged 3 commits intoy-scope:mainfrom
gibber9809:timestamp-parser-ignore-quotes
Jan 8, 2026
Merged

feat(clp-s::timestamp_parser): Add support for ignoring quotes in quoted timestamp patterns.#1847
gibber9809 merged 3 commits intoy-scope:mainfrom
gibber9809:timestamp-parser-ignore-quotes

Conversation

@gibber9809
Copy link
Contributor

@gibber9809 gibber9809 commented Jan 7, 2026

Description

This PR changes the parsing behaviour of the timestamp parser to skip the beginning and end quote in a quoted timestamp pattern when parsing raw UTF-8 timestamps. This change will allow some of the code in #1788 to be simplified.

This change allows code that uses this library to handle UTF-8 and JSON literals in a simpler way. E.g., the pattern "\Y" can be used to parse both the JSON literal "2024" and the raw UTF-8 string 2024 by simply passing the right flags to timestamp_parser::parse_timestamp. The previous approach would have required library users to use a different timestamp pattern (here, \Y) for the UTF-8 case, which is cumbersome.

Checklist

  • The PR satisfies the contribution guidelines.
  • This is a breaking change and that has been indicated in the PR title, OR this isn't a
    breaking change.
  • Necessary docs have been updated, OR no docs need to be updated.

Validation performed

  • Updated tests to exercise all valid combinations of parsing quoted/unquoted content with quoted/unquoted patterns.

Summary by CodeRabbit

Release Notes

  • New Features

    • Added support for quoted timestamp patterns, allowing timestamps enclosed in quotation marks to be properly recognized and parsed.
    • Enhanced timestamp parsing to correctly interpret and handle JSON escape sequences and special characters.
  • Tests

    • Expanded test coverage to include verification of quoted timestamp patterns and multiple parsing scenarios, including edge cases with JSON escape sequences.

✏️ Tip: You can customize this high-level summary in your review settings.

@gibber9809 gibber9809 requested a review from a team as a code owner January 7, 2026 20:23
@coderabbitai
Copy link
Contributor

coderabbitai bot commented Jan 7, 2026

Walkthrough

The changes introduce support for quoted timestamp patterns in the timestamp parser. The TimestampPattern class now tracks whether a pattern is quoted via a new is_quoted_pattern flag. Pattern parsing logic is updated to handle quoted patterns by optionally skipping leading quotes, and test coverage is expanded to exercise both quoted and unquoted pattern combinations.

Changes

Cohort / File(s) Summary
Timestamp Pattern Infrastructure
components/core/src/clp_s/timestamp_parser/TimestampParser.hpp
Added public accessor is_quoted_pattern() const and expanded constructor to accept new is_quoted_pattern boolean parameter for tracking quoted pattern state
Quoted Pattern Parsing Logic
components/core/src/clp_s/timestamp_parser/TimestampParser.cpp
Renamed quoted_pattern flag to is_quoted_pattern and updated index calculations; modified parse_timestamp to conditionally skip leading quotes when pattern is quoted; updated pattern boundary validation to use pattern_end_idx
Test Coverage Expansion
components/core/src/clp_s/timestamp_parser/test/test_TimestampParser.cpp
Integrated Catch2 generators for multi-case test evaluation; added retrieval and testing of quoted timestamp patterns alongside default patterns; expanded test cases to cover JSON escape sequences with both quoted and unquoted pattern variants

Estimated code review effort

🎯 3 (Moderate) | ⏱️ ~20 minutes

🚥 Pre-merge checks | ✅ 2 | ❌ 1
❌ Failed checks (1 warning)
Check name Status Explanation Resolution
Docstring Coverage ⚠️ Warning Docstring coverage is 60.00% which is insufficient. The required threshold is 80.00%. Write docstrings for the functions missing them to satisfy the coverage threshold.
✅ Passed checks (2 passed)
Check name Status Explanation
Description Check ✅ Passed Check skipped - CodeRabbit’s high-level summary is enabled.
Title check ✅ Passed The title accurately reflects the main change: adding support for ignoring quotes in quoted timestamp patterns, which is the core functionality introduced in this PR.

✏️ Tip: You can configure your own custom pre-merge checks in the settings.

✨ Finishing touches
  • 📝 Generate docstrings

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.

❤️ Share

Comment @coderabbitai help to get the list of available commands and usage tips.

@gibber9809 gibber9809 changed the title feat(clp_s::timestamp-parser): Ignore quotes in quoted timestamp patterns when parsing raw UTF-8 timestamps. feat(clp-s::timestamp_parser): Ignore quotes in quoted timestamp patterns when parsing raw UTF-8 timestamps. Jan 7, 2026
Copy link
Contributor

@20001020ycx 20001020ycx left a comment

Choose a reason for hiding this comment

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

LGTM!

Copy link
Member

@LinZhihao-723 LinZhihao-723 left a comment

Choose a reason for hiding this comment

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

The change lgtm!
For the PR title, how about:

feat(clp-s::timestamp_parser): Add support for ignoring quotes in quoted timestamp patterns.

to make it a bit more concise

@gibber9809 gibber9809 changed the title feat(clp-s::timestamp_parser): Ignore quotes in quoted timestamp patterns when parsing raw UTF-8 timestamps. feat(clp-s::timestamp_parser): Add support for ignoring quotes in quoted timestamp patterns. Jan 8, 2026
@gibber9809 gibber9809 merged commit bab9d9a into y-scope:main Jan 8, 2026
27 checks passed
davidlion pushed a commit to davidlion/clp that referenced this pull request Jan 17, 2026
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.

3 participants