-
Notifications
You must be signed in to change notification settings - Fork 1.1k
parents are include in query params when parent is an object. #4760
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
…the annotated thing was an object
WalkthroughThis update modifies resource serialization by changing the Changes
📜 Recent review detailsConfiguration used: CodeRabbit UI 📒 Files selected for processing (1)
🚧 Files skipped from review as they are similar to previous changes (1)
🪧 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 (
|
…the annotated thing was an object
638ccfa
to
53afbff
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.
It doesn't really make sense to use objects as parameters here since they're statically scoped. It would be better to rewrite your class here as data object Api { val parent get() = V1 }
.
That being said, the added safety here is not unwelcome. Could you review the one comment and also run ./gradlew formatKotlin
to pass the lint check?
ktor-shared/ktor-resources/common/src/io/ktor/resources/serialization/ResourcesFormat.kt
Outdated
Show resolved
Hide resolved
…fix-query-params-resource
This is much better. Never occurred to me.
Seems to be running successfully without changes locally? I'm not able to see any CI runs to check why they are failing (I think the code review change may have inadvertently fixed) Also, I ran into this when using some changes similar to #4747 on my own project, would I be able to get a review on that? I would like to know if something like that would be considered to be merged, or if that's out of KTOR's scope and I should be working on a solution application-side... |
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. Sorry about the CI, we've been having some tests go flaky recently. I can re-run them.
Subsystem
Resources
Failing Test
This is a bug, the parent is part of the path. not the query.
This PR fixes it.