fix: set interpolation macro/filter titles to match the actual function name #74
Conversation
lmossman
changed the title
📝 Walkthrough📝 WalkthroughWalkthroughThe changes in the pull request involve significant updates to the Changes
Sequence Diagram(s)sequenceDiagram
participant Developer
participant AirbyteCDK
participant DeclarativeSchema
Developer->>AirbyteCDK: Define new declarative source
AirbyteCDK->>DeclarativeSchema: Validate schema
DeclarativeSchema-->>AirbyteCDK: Return validation results
AirbyteCDK-->>Developer: Notify of success or errors
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
🧹 Outside diff range and nitpick comments (8)
airbyte_cdk/sources/declarative/declarative_component_schema.yaml (8)
2793-2800: Thenow_utcmacro description could be more precise.The description could benefit from clarifying that it returns a datetime object in UTC timezone, which can be formatted using strftime. Also, should we mention that the time component includes microseconds precision? wdyt?
- description: Returns the current date and time in the UTC timezone. + description: Returns the current date and time as a datetime object in UTC timezone with microseconds precision. The returned object can be formatted using strftime.
2800-2807: Thetoday_utcmacro examples could be more comprehensive.The examples show basic usage but could be enhanced to demonstrate more date formatting options that users might need. What do you think about adding an example with different date separators?
examples: - "'{{ today_utc() }}' -> '2021-09-01'" - "'{{ today_utc().strftime('%Y/%m/%d')}}' -> '2021/09/01'" + - "'{{ today_utc().strftime('%Y-%m-%d')}}' -> '2021-09-01'" + - "'{{ today_utc().strftime('%d-%m-%Y')}}' -> '01-09-2021'"
Line range hint
2807-2818: Thetimestampmacro description could be clearer about timezone handling.The description mentions "If no timezone is specified, the string is interpreted as UTC" but doesn't explicitly state what happens with timezone-aware inputs. Should we clarify this? Also, the examples could show more timezone variations, wdyt?
description: | - Converts a number or a string representing a datetime (formatted as ISO8601) to a timestamp. If the input is a number, it is converted to an int. If no timezone is specified, the string is interpreted as UTC. + Converts a number or a string representing a datetime (formatted as ISO8601) to a Unix timestamp (seconds since epoch). + - If the input is a number, it is converted to an integer + - If the input is a string without timezone, it is interpreted as UTC + - If the input is a string with timezone, the timezone offset is applied before conversion
2818-2826: Themaxmacro description has a typo.There's a small typo in the description - "or or". Also, should we add examples with different data types to show type handling? wdyt?
- description: Returns the largest object of a iterable, or or two or more arguments. + description: Returns the largest object of an iterable, or two or more arguments. examples: - "'{{ max(2, 3) }}' -> 3" - "'{{ max([2, 3]) }}' -> 3" + - "'{{ max('a', 'b', 'c') }}' -> 'c'" + - "'{{ max([1.5, 2.1, 0.3]) }}' -> 2.1"
Line range hint
2826-2836: Theday_deltamacro has inconsistent datetime format in examples.The examples show inconsistent datetime formats (some with 'T', some with ':'). Should we standardize them for clarity? Also, the description could mention the default format when none is specified.
examples: - - "'{{ day_delta(1) }}' -> '2021-09-02T00:00:00.000000+0000'" - - "'{{ day_delta(-1) }}' -> '2021-08-31:00:00.000000+0000'" + - "'{{ day_delta(1) }}' -> '2021-09-02T00:00:00.000000+0000'" + - "'{{ day_delta(-1) }}' -> '2021-08-31T00:00:00.000000+0000'" - "'{{ day_delta(25, format='%Y-%m-%d') }}' -> '2021-09-02'"
Line range hint
2836-2845: Thedurationmacro has a typo in description.There's a typo in "duratioin". Also, should we add more examples with different duration components? wdyt?
- description: Converts an ISO8601 duratioin to datetime.timedelta. + description: Converts an ISO8601 duration to datetime.timedelta. examples: - "'{{ duration('P1D') }}' -> '1 day, 0:00:00'" - "'{{ duration('P6DT23H') }}' -> '6 days, 23:00:00'" - "'{{ (now_utc() - duration('P1D')).strftime('%Y-%m-%dT%H:%M:%SZ') }}' -> '2021-08-31T00:00:00Z'" + - "'{{ duration('PT1H30M') }}' -> '0 days, 1:30:00'" + - "'{{ duration('P1Y2M') }}' -> '428 days, 0:00:00'"
Line range hint
2857-2867: Thehashfilter could use more hash type examples.The description mentions "Valid hash type" but doesn't list the supported types. Should we add examples with other common hash types like SHA-256? wdyt?
examples: - "{{ 'Test client_secret' | hash() }} -> '3032d57a12f76b61a820e47b9a5a0cbb'" - "{{ 'Test client_secret' | hash('md5') }} -> '3032d57a12f76b61a820e47b9a5a0cbb'" - "{{ 'Test client_secret' | hash('md5', salt='salt') }} -> '5011a0168579c2d94cbbe1c6ad14327c'" + - "{{ 'Test client_secret' | hash('sha256') }} -> 'a88c23aa6a5e46d72bc6c5a737a848f3ca15c236872d5b05de47c784cf3aeb41'"
Line range hint
2879-2892: Thestringandregex_searchfilters could use more examples.These filters might benefit from more diverse examples showing edge cases. For instance,
stringcould show handling of special characters, andregex_searchcould demonstrate more complex patterns. What do you think?examples: - '{{ 1 | string }} -> "1"' - '{{ ["hello", "world" | string }} -> "["hello", "world"]"' + - '{{ None | string }} -> "None"' + - '{{ true | string }} -> "True"' examples: - '{{ "goodbye, cruel world" | regex_search("goodbye,\s(.*)$") }} -> "cruel world"' + - '{{ "[email protected]" | regex_search("(.+)@.+") }} -> "user"' + - '{{ "ID: 12345" | regex_search("ID:\s*(\d+)") }} -> "12345"'
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Outside diff range and nitpick comments (1)
airbyte_cdk/sources/declarative/declarative_component_schema.yaml (1)
Line range hint
2857-2886: The filter titles now correctly match their function names. Would you consider adding more examples?The documentation looks great! To make it even more helpful for users, what do you think about adding examples for edge cases? For instance:
- For
hash: Example with different hash types (sha256, etc.)- For
base64encode: Example with special characters- For
regex_search: Example with multiple capture groups
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
📒 Files selected for processing (1)
airbyte_cdk/sources/declarative/declarative_component_schema.yaml(5 hunks)
🔇 Additional comments (2)
airbyte_cdk/sources/declarative/declarative_component_schema.yaml (2)
Line range hint 2793-2845: LGTM! The macro titles now match their function names.
The documentation for each macro is well-structured with clear descriptions, arguments, return types, and helpful examples. This aligns perfectly with the PR objective of making titles match the actual function names.
Line range hint 2793-2886: Verify the usage of these macros and filters in the codebase
Let's ensure these changes don't affect existing usage patterns.
|
Happy to merge as is, but what's up with Chargebee here? |
|
@natikgadzhi No idea - my PR did not touch it |
|
@lmossman good to merge — @maxi297 works on chargebee problem separately. @aaronsteers @ChristoGrab we ready to merge and ship the CDK version? Will release work and publish to pypi, and publish SDM alongside it? ;) |
|
For chargebee, the fixes will be available on airbytehq/airbyte#48510 so that's fine |
What
I am currently working on implementing the UI changes for this issue, to add a pop-up menu listing the various interpolation variables and functions that are available to the user.
Here is what the draft implementation currently looks like for
variables:The contents of this menu come from the interpolation section of the declarative component schema.
However, there is a problem: the
titles of the macros and filters in that section do not match the actual function names, and the function names don't appear anywhere else in that section. So, the UI doesn't know what text to actually insert into the input when the user selects one of these.How
This PR solves the issue by renaming the titles to match the actual function names themselves.
The UI can then use the title to populate the list and also to insert directly into the input upon selection, just like it does with the variables.
Open Questions
Summary by CodeRabbit
New Features
Improvements