fix: make new datetime parse functions more permissive #296
Conversation
📝 WalkthroughWalkthroughThe pull request introduces enhancements to datetime handling utilities in the Airbyte CDK. The modifications focus on improving datetime parsing and formatting flexibility, with new functions like Changes
Sequence DiagramsequenceDiagram
participant Input as Datetime String
participant Parser as ab_datetime_parse()
participant TryParser as ab_datetime_try_parse()
participant Formatter as ab_datetime_format()
Input->>TryParser: Attempt parsing
alt Parsing Successful
TryParser-->>Input: Return AirbyteDateTime
else Parsing Failed
TryParser-->>Input: Return None
end
Input->>Parser: Parse datetime
Parser-->>Input: Return AirbyteDateTime
Input->>Formatter: Format datetime
Formatter-->>Input: Return formatted string
Suggested reviewers
How do you feel about the proposed reviewers? Do you think there are others who might be relevant? Warning Review ran into problems🔥 ProblemsGit: Failed to clone repository. Please run the 📜 Recent review detailsConfiguration used: CodeRabbit UI 📒 Files selected for processing (1)
🚧 Files skipped from review as they are similar to previous changes (1)
⏰ Context from checks skipped due to timeout of 90000ms (9)
Thank you for using CodeRabbit. We offer it for free to the OSS community and would appreciate your support in helping us grow. If you find it useful, would you consider giving us a shout-out on your favorite social media? 🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
Note: Be mindful of the bot's finite context window. It's strongly recommended to break down tasks such as reading entire modules into smaller chunks. For a focused discussion, use review comments to chat about specific files and their changes, instead of using the PR comments. CodeRabbit Commands (Invoked using PR comments)
Other keywords and placeholders
CodeRabbit Configuration File (
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (6)
unit_tests/utils/test_datetime_helpers.py (3)
54-68: Consider adding more edge cases to test datetime parsing, wdyt?The test cases cover basic scenarios well, but we might want to add tests for:
- Timestamps with different precisions (seconds vs milliseconds)
- Mixed date separators (e.g., "2023-03/14")
- Various timezone formats (EDT, PST, etc.)
🧰 Tools
🪛 GitHub Actions: Linters
[warning] File requires formatting. Code does not conform to Ruff's formatting standards.
114-120: Should we remove the commented-out test cases?These test cases appear to be obsolete with the new permissive parsing approach. Let's keep the codebase clean by removing them, wdyt?
🧰 Tools
🪛 GitHub Actions: Linters
[warning] File requires formatting. Code does not conform to Ruff's formatting standards.
127-133: Consider making the try_parse assertion more explicit, wdyt?The current assertion combines two conditions. We could make it more explicit and easier to debug:
- assert ab_datetime_try_parse(input_value) and ab_datetime_try_parse(input_value) == dt + parsed = ab_datetime_try_parse(input_value) + assert parsed is not None, "Expected successful parsing" + assert parsed == dt, "Parsed value doesn't match expected"🧰 Tools
🪛 GitHub Actions: Linters
[warning] File requires formatting. Code does not conform to Ruff's formatting standards.
airbyte_cdk/utils/datetime_helpers.py (3)
364-372: Would you like to structure the docstring formats more clearly?Consider using bullet points with examples for each supported format, wdyt?
Handles: - - ISO8601/RFC3339 format strings (with ' ' or 'T' delimiter) - - Unix timestamps (as integers or strings) - - Date-only strings (YYYY-MM-DD) - - Timezone-aware formats (+00:00 for UTC, or ±HH:MM offset) - - Anything that can be parsed by `dateutil.parser.parse()` + - ISO8601/RFC3339 format strings: + - With 'T' delimiter: "2023-03-14T15:09:26Z" + - With space delimiter: "2023-03-14 15:09:26Z" + - Unix timestamps: + - As integer: 1678806566 + - As string: "1678806566" + - Date-only strings: "2023-03-14" (assumes 00:00:00 UTC) + - Timezone-aware formats: + - UTC: "+00:00" or "Z" + - Offset: "±HH:MM" + - Any format supported by dateutil.parser.parse()
422-424: Consider strengthening the time-only validation, wdyt?The current check might have edge cases. Here's a more robust approach:
- if ":" in dt_str and dt_str.count("-") < 2 and dt_str.count("/") < 2: + if (":" in dt_str and + all(sep not in dt_str or dt_str.count(sep) < 2 + for sep in ["-", "/", "."])):This would catch more variations of time-only strings while still allowing valid datetime formats.
Line range hint
464-499: Should we add format string validation?Consider validating the format string to catch invalid patterns early, wdyt?
if format: + try: + # Validate format string with a known datetime + datetime.now().strftime(format) + except ValueError as e: + raise ValueError(f"Invalid format string: {e}") return dt.strftime(format)
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
airbyte_cdk/utils/datetime_helpers.py(8 hunks)unit_tests/utils/test_datetime_helpers.py(3 hunks)
🧰 Additional context used
🪛 GitHub Actions: Linters
unit_tests/utils/test_datetime_helpers.py
[warning] File requires formatting. Code does not conform to Ruff's formatting standards.
⏰ Context from checks skipped due to timeout of 90000ms (9)
- GitHub Check: Check: 'source-pokeapi' (skip=false)
- GitHub Check: Check: 'source-the-guardian-api' (skip=false)
- GitHub Check: Check: 'source-shopify' (skip=false)
- GitHub Check: Check: 'source-hardcoded-records' (skip=false)
- GitHub Check: Pytest (All, Python 3.12, Ubuntu)
- GitHub Check: Pytest (All, Python 3.11, Ubuntu)
- GitHub Check: Analyze (python)
- GitHub Check: Pytest (All, Python 3.10, Ubuntu)
- GitHub Check: Pytest (Fast)
🔇 Additional comments (1)
airbyte_cdk/utils/datetime_helpers.py (1)
141-147: LGTM! Good forward-thinking with the docstring.The implementation and documentation clearly explain the current behavior and potential future changes.
|
/autofix
|
aaronsteers
changed the title
1 participant
Summary by CodeRabbit
New Features
ab_datetime_try_parsefunction for flexible datetime parsing.to_datetimemethod for convertingAirbyteDateTimeto standard datetime.Bug Fixes