feat: add formatting for epoch_microseconds #388
Conversation
Signed-off-by: Artem Inzhyyants <[email protected]>
Signed-off-by: Artem Inzhyyants <[email protected]>
📝 WalkthroughWalkthroughThis update introduces support for a new datetime format ( Changes
Possibly related PRs
Suggested reviewers
Would these suggestions work for you? wdyt? 📜 Recent review detailsConfiguration used: CodeRabbit UI 📒 Files selected for processing (5)
💤 Files with no reviewable changes (1)
✅ Files skipped from review due to trivial changes (3)
🚧 Files skipped from review as they are similar to previous changes (1)
⏰ Context from checks skipped due to timeout of 90000ms (9)
🪧 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)
51-52: Format implementation for %epoch_microseconds looks good.The implementation correctly converts a datetime to microseconds since the epoch. This aligns with the implementation in macros.py.
Since both "%ms" and "%epoch_microseconds" handle timestamps but at different granularities, it might be helpful to add a brief comment explaining their difference, 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/interpolation/macros.py(1 hunks)unit_tests/sources/declarative/interpolation/test_macros.py(1 hunks)
⏰ Context from checks skipped due to timeout of 90000ms (3)
- GitHub Check: Check: 'source-shopify' (skip=false)
- GitHub Check: Pytest (All, Python 3.11, Ubuntu)
- GitHub Check: Pytest (All, Python 3.10, Ubuntu)
🔇 Additional comments (3)
airbyte_cdk/sources/declarative/interpolation/macros.py (1)
176-177: Implementation looks good!The new format handling for "%epoch_microseconds" is correctly implemented, converting seconds to microseconds by multiplying by 1,000,000. This matches the implementation in the DatetimeParser class.
A tiny enhancement could be adding a brief comment before this branch to clarify its purpose (similar to the comment before the "%s" branch), wdyt?
unit_tests/sources/declarative/interpolation/test_macros.py (1)
82-82: Test update correctly verifies the new format.The test case has been properly updated to use the new "%epoch_microseconds" format while keeping the same expected result, which makes sense since this is just renaming the format but keeping the same conversion logic.
airbyte_cdk/sources/declarative/datetime/datetime_parser.py (1)
32-33: Parse implementation for %epoch_microseconds looks good.The implementation correctly adds microseconds to the UNIX epoch to parse a timestamp in microseconds. This is consistent with how other formats are handled.
artem1205
changed the title
Signed-off-by: Artem Inzhyyants <[email protected]>
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (1)
unit_tests/sources/declarative/datetime/test_datetime_parser.py (1)
12-127: Consider adding edge cases for microsecond parsing?The changes look great! Would you consider adding a few edge cases for the microsecond timestamp parsing? For example, testing with very large or small values might help ensure robust handling of all possible inputs, wdyt?
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
unit_tests/sources/declarative/datetime/test_datetime_parser.py(1 hunks)
⏰ Context from checks skipped due to timeout of 90000ms (9)
- 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: SDM Docker Image Build
- 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 (4)
unit_tests/sources/declarative/datetime/test_datetime_parser.py (4)
12-13: Looking good! Type annotations and improved test identifiers.The parametrize decorator has been updated to remove the
test_nameparameter and add descriptive test IDs. The type annotations on the test function parameters also improve code readability. These changes will help with test clarity and maintainability.Also applies to: 58-67, 69-69
38-41: Great addition of microseconds timestamp test case!The test case for parsing timestamps in microseconds format is well implemented and aligns perfectly with the PR objective of adding support for
%epoch_microseconds.
94-97: Nice complementary test for formatting to microseconds.This test case properly verifies that formatting to microseconds works correctly, which complements the parsing test case added earlier. The expected output value is correct for the given input datetime.
75-76: Consistent improvements to format_datetime test structure.The changes to the test_format_datetime function match the pattern applied to test_parse_date, with the removal of the test_name parameter, addition of descriptive IDs, and type annotations. This consistency across test functions is excellent!
Also applies to: 114-122, 124-124
There was a problem hiding this comment.
It feels like there is a problem somewhere but I think it is in the formatting of epoch millis aka %ms. I'm kind of worried because it used in a couple connectors today (dixa, appcues, front, lever-hiring, linkedin-pages, mixmax...) and in most cases, it would mean that we send a epoch microsecond to the API instead and fixing that would be a breaking change
Signed-off-by: Artem Inzhyyants <[email protected]>
* main: feat: add formatting for epoch_microseconds (airbytehq#388) feat(low-code cdk): remove unnecessary manifest print (airbytehq#390)
What
Resolving airbytehq/airbyte#54151
How
add
%epoch_microsecondsto parser. Reason: concurrent curosor stateconverter uses parser to operate state valuesSummary by CodeRabbit
New Features
Tests