-
Notifications
You must be signed in to change notification settings - Fork 32
fix: update decoder title & descriptions for improved tooltip #434
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
fix: update decoder title & descriptions for improved tooltip #434
Conversation
📝 WalkthroughWalkthroughThis pull request updates the Changes
Suggested labels
Suggested reviewers
Tip ⚡🧪 Multi-step agentic review comment chat (experimental)
🪧 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 (
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Actionable comments posted: 0
🧹 Nitpick comments (1)
airbyte_cdk/sources/declarative/declarative_component_schema.yaml (1)
2373-2374: ZipfileDecoder Description TypoThe description currently reads:
Select 'ZIP'file" for response data that is returned as a zipfile. Requires specifying an inner data type/decoder to parse the unzipped data.
There appears to be an extra word or misplaced quotation mark with 'ZIP'file". Should we change it to something like:
Select 'ZIP' for response data returned as a zipfile, requiring an inner data type/decoder to parse the unzipped data?
wdyt?
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
airbyte_cdk/sources/declarative/declarative_component_schema.yaml(7 hunks)
⏰ Context from checks skipped due to timeout of 90000ms (5)
- 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)
🔇 Additional comments (6)
airbyte_cdk/sources/declarative/declarative_component_schema.yaml (6)
2195-2196: JsonDecoder Description UpdateThe updated description clearly instructs users to select 'JSON' when the response is a JSON object. This enhancement makes the tooltip much more informative. wdyt?
2205-2206: JsonlDecoder Description UpdateThe revised description now instructs users to select 'JSONL' if the response consists of JSON objects separated by new lines (
\n). It’s clearer and aligns with the intended usage for JSONL responses. wdyt?
2331-2332: IterableDecoder Description UpdateThis update explains that if the response comprises strings separated by new lines, the string will be wrapped into a JSON object with the
recordkey. The update adds clarity and consistency with the other decoders. wdyt?
2341-2342: XML Decoder Description UpdateThe new description concisely instructs users to select 'XML' for XML-formatted responses. It is clear and to the point. wdyt?
3280-3281: GzipDecoder Description AdditionThe new GzipDecoder entry is a helpful addition. The description clearly tells users to select 'gzip' when the response data is compressed with gzip and notes that an inner decoder must be specified. It’s both informative and consistent with the other decoders. wdyt?
3297-3298: CsvDecoder Description AdditionThe CsvDecoder now includes a clear title and a detailed description explaining that it should be used for CSV-formatted responses. It also provides default encoding and delimiter values. The explanation is both clear and useful. wdyt?
airbyte_cdk/sources/declarative/declarative_component_schema.yaml
Outdated
Show resolved
Hide resolved
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Actionable comments posted: 0
🧹 Nitpick comments (7)
airbyte_cdk/sources/declarative/declarative_component_schema.yaml (7)
2195-2196: Decoder Tooltip Update for JSON
The new title "JSON" along with the description "Select 'JSON' if the response is formatted as a JSON object." makes the purpose clear. Do you feel this succinct phrasing aligns with the UI guidelines and meets user expectations? wdyt?
2205-2206: Decoder Tooltip Update for JSON Lines
Updating the title to "JSON Lines" and clarifying that the response consists of JSON objects separated by new lines is a nice improvement. Have you verified that the inclusion of\nin the description will render as intended in the Builder UI? wdyt?
2331-2332: Decoder Tooltip Update for Iterable
The revised title "Iterable" and the description indicating the response consists of newline-separated strings—with the string subsequently wrapped into a JSON object under therecordkey—improve clarity. Do you think this change communicates the functionality unambiguously to users? wdyt?
2341-2342: Decoder Tooltip Update for XML
Changing the title to "XML" and updating the description to "Select 'XML' if the response consists of XML-formatted data." provides a cleaner presentation. Does this update effectively resolve the previous naming inconsistency, in your view? wdyt?
2373-2374: Decoder Tooltip Update for ZIP File
The update to "ZIP File" with its revised description clearly instructs users to select this option when the response data is a zipfile and highlights the requirement for specifying an inner decoder. Do you think this additional nuance will help avoid misconfiguration? wdyt?
3280-3281: Decoder Tooltip Update for gzip
The new title "gzip" and its corresponding description succinctly indicate that the response data is compressed using gzip and that an inner data type/decoder is required for decompression. Is using all lowercase for "gzip" intentional to match the ecosystem’s style, and does it feel consistent with other decoder names? wdyt?
3297-3298: Decoder Tooltip Update for CSV
The updated title "CSV" coupled with the detailed description—highlighting that CSV formatted data is expected and noting the default encoding ("utf-8") and delimiter (",")—adds useful clarity. Would you consider adding an example or additional context to further aid users, or is this level of detail sufficient? wdyt?
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
airbyte_cdk/sources/declarative/declarative_component_schema.yaml(9 hunks)
⏰ Context from checks skipped due to timeout of 90000ms (7)
- 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)
lmossman
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Changes LGTM
What
This PR adds titles/descriptions to decoders where missing and updates existing descriptions as necessary to more closely align with the Builder UI.
Current tooltip provides insufficient and incorrect data as not all decoders title/description fields are properly populated:
Summary by CodeRabbit
Documentation
New Features