feat: (low-code cdk) datetime format with milliseconds #369
Conversation
darynaishchenko
changed the title
darynaishchenko
changed the title
📝 WalkthroughWalkthroughThis PR updates the datetime handling capabilities by modifying the Changes
Sequence Diagram(s)Datetime Parsing FlowsequenceDiagram
participant U as User
participant DP as DatetimeParser
participant DS as datetime Library (strptime)
U->>DP: call parse(date, format)
DP->>DP: Check if format contains "%_ms"
DP->>DP: Replace "%_ms" with "%f" if needed
DP->>DS: Parse modified date string using strptime
DS-->>DP: Return datetime object
DP-->>U: Return parsed datetime
Datetime Formatting FlowsequenceDiagram
participant U as User
participant DP as DatetimeParser
participant DS as datetime Library (strftime)
U->>DP: call format(datetime, format)
DP->>DP: Check if format contains "%_ms"
DP->>DP: Replace "%_ms" with "%f" if needed
DP->>DS: Format datetime using strftime
DP->>DP: Calculate milliseconds from microsecond
DP->>DP: Replace microsecond part with calculated milliseconds
DP-->>U: Return formatted datetime string
Would you like any further adjustments or clarifications? Tip CodeRabbit's docstrings feature is now available as part of our Pro Plan! Simply use the command ✨ Finishing Touches
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 (1)
airbyte_cdk/sources/declarative/datetime/datetime_parser.py (1)
52-56: The formatting implementation works well but could be slightly clearer.The implementation correctly calculates milliseconds from microseconds and formats the output. However, the line 55 packs multiple operations into a single statement, which might be a bit challenging to read.
Would splitting this into multiple lines improve readability? Something like:
_format = format.replace("%_ms", "%f") milliseconds = int(dt.microsecond / 1000) formatted_with_micros = dt.strftime(_format) micros_part = dt.strftime("%f") milliseconds_formatted = f"{milliseconds:03d}" formatted_dt = formatted_with_micros.replace(micros_part, milliseconds_formatted)This is just a suggestion for readability, the current implementation works correctly. wdyt?
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (3)
airbyte_cdk/sources/declarative/datetime/datetime_parser.py(2 hunks)airbyte_cdk/sources/declarative/declarative_component_schema.yaml(2 hunks)unit_tests/sources/declarative/datetime/test_datetime_parser.py(2 hunks)
⏰ Context from checks skipped due to timeout of 90000ms (8)
- GitHub Check: Check: 'source-pokeapi' (skip=false)
- GitHub Check: Check: 'source-amplitude' (skip=false)
- GitHub Check: Check: 'source-shopify' (skip=false)
- GitHub Check: Check: 'source-hardcoded-records' (skip=false)
- GitHub Check: Pytest (All, Python 3.11, Ubuntu)
- GitHub Check: Pytest (All, Python 3.10, Ubuntu)
- GitHub Check: Pytest (Fast)
- GitHub Check: Analyze (python)
🔇 Additional comments (6)
unit_tests/sources/declarative/datetime/test_datetime_parser.py (2)
53-58: Great test case for the new millisecond format!The test case properly validates that parsing a datetime string with milliseconds using the new
%_msformat works correctly. It correctly expects a datetime object with 640000 microseconds (equivalent to 640 milliseconds).
100-105: Test case looks good for formatting with milliseconds!This test complements the parsing test by verifying that formatting a datetime object with the
%_msspecifier correctly produces a string with milliseconds. Good to see both directions being tested.airbyte_cdk/sources/declarative/declarative_component_schema.yaml (2)
847-847: Documentation for the new format specifier looks good!Clear documentation for the new
%_msmillisecond format specifier. This will help users understand how to format datetime strings with milliseconds.
2405-2405: Documentation is consistent across the file.Good job maintaining consistency by adding the same documentation in both relevant sections of the schema file.
airbyte_cdk/sources/declarative/datetime/datetime_parser.py (2)
34-35: Implementation for parsing milliseconds looks good!The approach to replace
%_mswith%ffor parsing is elegant and leverages the existing microsecond parsing capability.
52-56:✅ Verification successful
Consider edge cases with multiple %_ms occurrences.
The current implementation would handle multiple
%_msoccurrences in the format string, but it might not replace all microsecond instances correctly since the replacement on line 55 would only affect the first occurrence of%f.If this is a supported use case, would it be worth handling multiple replacements? If not, perhaps add a comment noting this limitation? wdyt?
🏁 Script executed:
#!/bin/bash # Check if there are any format strings with multiple %_ms occurrences in tests or examples grep -r "%_ms.*%_ms" --include="*.py" --include="*.yaml" .Length of output: 60
Subject: Clarification on
%_msReplacement Behavior in DateTime ParserI checked for multiple
%_msoccurrences in tests and examples, and nothing was found. Additionally, since Python’sstr.replacereplaces all matches by default, the current implementation correctly handles multiple occurrences. Would you like to add a clarifying comment documenting that behavior, or are we comfortable leaving it as is? wdyt?
darynaishchenko
deleted the
daryna/low-code/milliseconds-datetime-format
branch
* main: fix: update cryptography package to latest version to address CVE (airbytehq#377) fix: (CDK) (HttpRequester) - Make the `HttpRequester.path` optional (airbytehq#370) feat: improved custom components handling (airbytehq#350) feat: add microseconds timestamp format (airbytehq#373) fix: Replace Unidecode with anyascii for permissive license (airbytehq#367) feat: add IncrementingCountCursor (airbytehq#346) feat: (low-code cdk) datetime format with milliseconds (airbytehq#369) fix: (CDK) (AsyncRetriever) - Improve UX on variable naming and interpolation (airbytehq#368) fix: (CDK) (AsyncRetriever) - Add the `request` and `response` to each `async` operations (airbytehq#356) fix: (CDK) (ConnectorBuilder) - Add `auxiliary requests` to slice; support `TestRead` for AsyncRetriever (part 1/2) (airbytehq#355) feat(concurrent perpartition cursor): Add parent state updates (airbytehq#343) fix: update csv parser for builder compatibility (airbytehq#364) feat(low-code cdk): add interpolation for limit field in Rate (airbytehq#353) feat(low-code cdk): add AbstractStreamFacade processing as concurrent streams in declarative source (airbytehq#347) fix: (CDK) (CsvParser) - Fix the `\\` escaping when passing the `delimiter` from Builder's UI (airbytehq#358) feat: expose `str_to_datetime` jinja macro (airbytehq#351) fix: update CDK migration for 6.34.0 (airbytehq#348) feat: Removes `stream_state` interpolation from CDK (airbytehq#320) fix(declarative): Pass `extra_fields` in `global_substream_cursor` (airbytehq#195) feat(concurrent perpartition cursor): Refactor ConcurrentPerPartitionCursor (airbytehq#331) feat(HttpMocker): adding support for PUT requests and bytes responses (airbytehq#342) chore: use certified source for manifest-only test (airbytehq#338) feat: check for request_option mapping conflicts in individual components (airbytehq#328) feat(file-based): sync file acl permissions and identities (airbytehq#260) fix: (CDK) (Connector Builder) - refactor the `MessageGrouper` > `TestRead` (airbytehq#332) fix(low code): Fix missing cursor for ClientSideIncrementalRecordFilterDecorator (airbytehq#334) feat(low-code): Add API Budget (airbytehq#314) chore(decoder): clean decoders and make csvdecoder available (airbytehq#326)
What
https://github.com/airbytehq/airbyte-internal-issues/issues/11527
How
Added new custom placeholder for milliseconds
%_ms. To process it%f(microseconds) is used (seeairbyte_cdk/sources/declarative/datetime/datetime_parser.py).Summary by CodeRabbit