-
Notifications
You must be signed in to change notification settings - Fork 1.2k
KTOR-9037 Make formData inline function #5185
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
base: main
Are you sure you want to change the base?
Conversation
WalkthroughAdded a public no-arg constructor and a build() method to FormBuilder, introduced multiple append() overloads and an inline formData(builder) builder API, updated FormDataContent constructor/signatures, and applied Changes
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes
Pre-merge checks and finishing touches❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
✨ Finishing touches
🧪 Generate unit tests (beta)
📜 Recent review detailsConfiguration used: CodeRabbit UI Review profile: CHILL Plan: Pro 📒 Files selected for processing (3)
🚧 Files skipped from review as they are similar to previous changes (2)
🧰 Additional context used🧠 Learnings (2)📚 Learning: 2025-05-14T18:05:02.321ZApplied to files:
📚 Learning: 2025-06-23T12:49:56.883ZApplied to files:
🔇 Additional comments (2)
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. Comment |
c07f996 to
2b93efe
Compare
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)
ktor-client/ktor-client-core/common/src/io/ktor/client/request/forms/formDsl.kt (1)
89-90: Document the builder contract on the inline overload.We invoke
blockexactly once here. Adding the sameExperimentalContractsopt-in +contract { callsInPlace(block, InvocationKind.EXACTLY_ONCE) }used elsewhere tightens the DSL guarantees and keeps the new inline overload aligned with the rest of the API surface.- public inline fun formData(block: FormBuilder.() -> Unit): List<PartData> = - formData(*FormBuilder().apply(block).build().toTypedArray()) +@OptIn(ExperimentalContracts::class) +public inline fun formData(block: FormBuilder.() -> Unit): List<PartData> { + contract { callsInPlace(block, InvocationKind.EXACTLY_ONCE) } + return formData(*FormBuilder().apply(block).build().toTypedArray()) +}
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (3)
ktor-client/ktor-client-core/api/ktor-client-core.api(2 hunks)ktor-client/ktor-client-core/api/ktor-client-core.klib.api(3 hunks)ktor-client/ktor-client-core/common/src/io/ktor/client/request/forms/formDsl.kt(3 hunks)
🧰 Additional context used
🧠 Learnings (2)
📚 Learning: 2025-05-14T18:05:02.321Z
Learnt from: bjhham
Repo: ktorio/ktor PR: 4855
File: ktor-server/ktor-server-plugins/ktor-server-di/api/ktor-server-di.klib.api:334-336
Timestamp: 2025-05-14T18:05:02.321Z
Learning: Breaking changes in constructor parameter order are acceptable for the ktor-server-di module when the code hasn't been released yet, as confirmed by the development team.
Applied to files:
ktor-client/ktor-client-core/common/src/io/ktor/client/request/forms/formDsl.ktktor-client/ktor-client-core/api/ktor-client-core.klib.api
📚 Learning: 2025-06-23T12:49:56.883Z
Learnt from: CR
Repo: ktorio/ktor PR: 0
File: CLAUDE.md:0-0
Timestamp: 2025-06-23T12:49:56.883Z
Learning: Binary compatibility is enforced using the binary-compatibility-validator plugin.
Applied to files:
ktor-client/ktor-client-core/api/ktor-client-core.klib.api
🔇 Additional comments (2)
ktor-client/ktor-client-core/api/ktor-client-core.klib.api (2)
891-907: The review comment is incorrect; FormBuilder is already correctly annotated.The source code in
ktor-client/ktor-client-core/common/src/io/ktor/client/request/forms/formDsl.ktalready implements what the review suggests:
- Constructor:
@PublishedApi internal constructor()- build() method:
@PublishedApi internal fun build()The klib.api dump correctly exposes these members because
@PublishedApi internalmeans "internal but part of the public API for inliners." This is the intended design and causes no external compatibility issues since only the inlineformData()function usesFormBuilder()directly.Likely an incorrect or invalid review comment.
1531-1531: Kotlin/Native ABI incompatibility confirmed; verify release status before proceeding.Web search confirms your concern: changing a function from non-inline to inline is a binary-breaking change for Kotlin/Native klib consumers, as previously compiled call sites expect the original callable symbol, and making the function inline removes/changes that emitted symbol.
However, referencing Ktor team precedent: breaking changes to unreleased code are acceptable per prior PRs (#4916, #4855, #4860). If ktor-client-core hasn't been released yet, this change may be acceptable with a corresponding version bump. Confirm release status; if released, consider preserving a non-inline forwarding symbol to maintain binary compatibility.
2b93efe to
c9fcb5c
Compare
Subsystem
ktor-client-core
Motivation
KTOR-9037 Multipart/form-data: Make
formData's block inlineSolution
Add the
inlinemodifier to the function