Refactor DateScrubber to use datetime format strings internally #207
Conversation
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
- Uses datetime.strptime() for robust date parsing instead of complex regex patterns - Internal implementation now uses readable format strings like %Y%m%d_%H%M%S - External API maintains backward compatibility with regex patterns - Added support for YYYYMMDD_HHMMSS format (20250527_125703) from issue #124 - Easier to maintain: adding new date formats now requires only datetime format strings - All existing functionality preserved, all tests pass Related to #124 🤖 Generated with [Claude Code](https://claude.ai/code) Co-Authored-By: Claude <[email protected]>
Reviewer's GuideRefactored DateScrubber to leverage datetime format strings and parsing internally while preserving the external regex-based API, and added support for the new YYYYMMDD_HHMMSS format. Sequence Diagram for DateScrubber.get_scrubber_for()sequenceDiagram
participant Caller
participant DS_Static as DateScrubber (Static)
participant DT as datetime
participant DS_Instance as DateScrubber (Instance)
Caller->>DS_Static: get_scrubber_for(example)
DS_Static->>DS_Static: _get_internal_formats()
activate DS_Static
DS_Static-->>DS_Static: internal_formats_list
deactivate DS_Static
loop For each format in internal_formats_list
DS_Static->>DT: strptime(example, date_format)
alt strptime success
DT-->>DS_Static: parsed_datetime_object
DS_Static->>DS_Instance: new DateScrubber(date_format)
activate DS_Instance
DS_Instance-->>DS_Static: scrubber_instance
deactivate DS_Instance
DS_Static-->>Caller: scrubber_instance.scrub (callable)
Note right of Caller: Returns the scrub method
else strptime failure (ValueError)
DT-->>DS_Static: ValueError
end
end
Note over DS_Static: If loop completes, no match was found.
DS_Static->>DS_Static: get_supported_formats()
activate DS_Static
DS_Static-->>DS_Static: supported_formats_for_error_msg
deactivate DS_Static
DS_Static->>Caller: raise Exception("No match found...")
Sequence Diagram for DateScrubber.get_supported_formats()sequenceDiagram
participant Caller
participant DS_Static as DateScrubber (Static)
participant DS_Instance as DateScrubber (Instance)
Caller->>DS_Static: get_supported_formats()
DS_Static->>DS_Static: _get_internal_formats()
activate DS_Static
DS_Static-->>DS_Static: internal_formats_list
deactivate DS_Static
loop For each (date_format, _, display_examples) in internal_formats_list
DS_Static->>DS_Instance: new DateScrubber(date_format)
activate DS_Instance
Note over DS_Instance: __init__ calls _convert_format_to_regex(date_format)
DS_Instance-->>DS_Static: scrubber_instance (self.date_regex is now set)
deactivate DS_Instance
DS_Static->>DS_Instance: Get date_regex from scrubber_instance
DS_Instance-->>DS_Static: regex_pattern
DS_Static->>DS_Static: Add (regex_pattern, display_examples) to formats list
end
DS_Static-->>Caller: formats_list
File-Level Changes
Tips and commandsInteracting with Sourcery
Customizing Your ExperienceAccess your dashboard to:
Getting Help
|
![sourcery-ai[bot]](https://avatars.githubusercontent.com/in/48477?s=60&v=4){kind=small}
There was a problem hiding this comment.
Hey @nitsanavni - I've reviewed your changes - here's some feedback:
- The
%zmapping currently only matches offsets like+0300but your display examples use+03:00; consider updating the regex to accept the colon-separated form as well. - Using simple string replacements in
_convert_format_to_regexcan mis-replace overlapping tokens (e.g.%mvs%M); consider a more robust tokenization or regex-based substitution approach. - You rebuild the regex on every
DateScrubberinstantiation—caching the compiled patterns per format could significantly reduce redundant work.
Here's what I looked at during the review
- 🟡 General issues: 1 issue found
- 🟢 Testing: all looks good
- 🟢 Documentation: all looks good
Help me be more useful! Please click 👍 or 👎 on each comment and I'll use the feedback to improve your reviews.
| escaped_placeholder = re.escape(placeholder) | ||
| regex_pattern = regex_pattern.replace(escaped_placeholder, regex) | ||
|
|
||
| return regex_pattern |
There was a problem hiding this comment.
suggestion (bug_risk): Regex patterns are not anchored, may match substrings unexpectedly
Anchor the pattern with ^ and $, or use word boundaries, to ensure only complete date tokens are matched.
Suggested change
| return regex_pattern | |
| # Anchor the pattern to match the entire string | |
| return f"^{regex_pattern}$" |
|
|
||
| # Replace format codes with regex patterns first | ||
| regex_pattern = date_format | ||
| for format_code, regex in format_to_regex.items(): |
There was a problem hiding this comment.
suggestion (code-quality): Remove unnecessary calls to dict.items when the values are not used (remove-dict-items)
Suggested change
| for format_code, regex in format_to_regex.items(): | |
| for format_code in format_to_regex: |
5 tasks
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Summary
• Refactored DateScrubber to use Python's
datetime.strptime()internally for robust date parsing• Internal implementation now uses readable datetime format strings like
%Y%m%d_%H%M%Sinstead of complex regex patterns• External API maintains full backward compatibility - still returns regex patterns
• Added support for YYYYMMDD_HHMMSS format (
20250527_125703) from issue #124Key Benefits
• Maintainable: Adding new date formats now requires only datetime format strings
• Robust: Leverages Python's built-in datetime parsing for validation
• Backward Compatible: External API unchanged, all existing tests pass
• Developer Friendly: Self-documenting format strings vs complex regex
Test plan
20250527_125703works correctlyExample
Before:
[12]\d{3}[01]\d[0-3]\d_[0-2]\d[0-5]\d[0-5]\dAfter:
%Y%m%d_%H%M%S(internally) → still generates regex for external APIRelated to #124
🤖 Generated with Claude Code
Summary by Sourcery
Refactor DateScrubber to leverage datetime.strptime with format strings for robust parsing, introduce a conversion utility to maintain regex-based scrubbing and backward compatibility, and add support for the YYYYMMDD_HHMMSS format.
New Features:
Enhancements:
Tests: