-
Notifications
You must be signed in to change notification settings - Fork 345
update: mention the Unix domain socket functionality in making requests #644
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
Conversation
Important Review skippedAuto reviews are disabled on base/target branches other than the default branch. Please check the settings in the CodeRabbit UI or the You can disable this status message by setting the WalkthroughDocumentation for Ktor client features was updated to clarify and expand content related to Unix domain socket support. New sections and examples were added to both the DefaultRequest plugin and client request documentation, detailing usage with Unix sockets in the CIO engine. Minor formatting and wording improvements were also made. Changes
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. 🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
SupportNeed help? Create a ticket on our support page for assistance with any issues or questions. 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 (2)
topics/client-default-request.md (1)
103-122
: Specify explicit ID and improve phrasing for clarity.Add an explicit
id
to the### Unix domain sockets
heading for reliable fragment linking and reword theunixSocket
invocation sentence for readability:- ### Unix domain sockets + ### Unix domain sockets {id="unix-domain-sockets"} - To do that, pass a `unixSocket` call with the path to the socket to the `defaultRequest` function, + To configure this, call the `unixSocket` function within the `defaultRequest` block, passing the path to the socket:topics/client-requests.md (1)
123-154
: Add explicit ID to heading for cross-document linking.Ensure the link from the DefaultRequest docs resolves correctly by giving the heading a matching ID:
- ## Specify a Unix domain socket + ## Specify a Unix domain socket {id="specify-a-unix-domain-socket"}
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
topics/client-default-request.md
(3 hunks)topics/client-requests.md
(3 hunks)
🧰 Additional context used
🪛 markdownlint-cli2 (0.17.2)
topics/client-default-request.md
37-37: Link fragments should be valid
null
(MD051, link-fragments)
🪛 LanguageTool
topics/client-default-request.md
[style] ~112-~112: Consider a more expressive alternative.
Context: ...lt request with a socket parameter. To do that, pass a unixSocket
call with the...
(DO_ACHIEVE)
🔇 Additional comments (3)
topics/client-default-request.md (2)
25-26
: Approve phrasing for installation instructions.The updated installation instructions are clear and concise.
37-37
: Approve phrasing for defaultRequest invocation.The revised sentence clearly guides the user to use
defaultRequest
and link to the configuration section appropriately.topics/client-requests.md (1)
19-19
: Approve addition of Unix domain socket bullet.Listing Unix domain sockets as a configurable parameter aligns with related docs.
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.
Hey @zamulla, thank you for the contribution!
Please check the comments
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 (2)
topics/client-default-request.md (2)
103-107
: Nitpick: Admonition formatting consistency. The blockquote and{style="note"}
directive should be adjacent for correct rendering. For example:- > Unix domain sockets are supported only in the CIO engine, for Ktor server as well as Ktor client. - > - {style="note"} + > Unix domain sockets are supported only in the CIO engine, for Ktor server as well as Ktor client. + {style="note"}
109-114
: Nitpick: Simplify phrasing for clarity. The sentence is a bit wordy. Consider:- To do that, pass a `unixSocket` call with the path to the socket to the `defaultRequest` function, + To configure a default socket path, call `unixSocket("/path/to/socket")` inside the `defaultRequest` block,
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
topics/client-default-request.md
(3 hunks)topics/client-requests.md
(3 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
- topics/client-requests.md
🧰 Additional context used
🪛 markdownlint-cli2 (0.17.2)
topics/client-default-request.md
37-37: Link fragments should be valid
null
(MD051, link-fragments)
🪛 LanguageTool
topics/client-default-request.md
[style] ~112-~112: Consider a more expressive alternative.
Context: ...lt request with a socket parameter. To do that, pass a unixSocket
call with the...
(DO_ACHIEVE)
🔇 Additional comments (3)
topics/client-default-request.md (3)
25-26
: Improve installation instruction clarity. Changed ellipses to a colon for consistency and clearer guidance on installing the plugin.
37-38
: Approve rephrased function invocation guidance. The new wording distinguishes installing the plugin versus configuring requests, improving readability.
115-123
: Example snippet looks correct. The code sample clearly shows how to set up and use a Unix domain socket within thedefaultRequest
configuration and make a subsequent request.
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.
Thanks for picking this up @zamulla 🙏
The content looks good, it's just that some of it seems misplaced (see comments).
Could you add a small section about this update in the whats-new-320.md file as well?
Co-authored-by: Vik Nikolova <[email protected]>
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.
Looks good 👌
Co-authored-by: Vik Nikolova <[email protected]>
KTOR-8509