-
Notifications
You must be signed in to change notification settings - Fork 2k
Runtime improvements #9722
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
Runtime improvements #9722
Conversation
|
Looks good. Let's review and merge please :) |
|
|
||
| get("/json") { | ||
| call.respondText(Json.encodeToString(Message("Hello, world!")), ContentType.Application.Json) | ||
| call.respondText(jsonResponse, ContentType.Application.Json) |
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, I was just making some updates for Ktor and I noticed this might be in violation of one of the test rules:
For each request, an object mapping the key message to Hello, World! must be instantiated.
Is this not applicable here, or should I change it?
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.
@bjhham I just checked, you are correct, I missed this rule. We should fix this.
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.
Bah, I'm going too quickly this morning. You are correct - you need to instantiate the object on every request with the json test.
Your plaintext is fine.
Unfortunately, Github won't let me un-merge - can you please open a second PR and resolve?
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.
Yep, I'll open one soon with a few tweaks to response streaming and an update to 3.1.2. as well. Thanks for updating it to 3.0 btw, I had intended to in 2024 but it slipped my mind.
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.
No worries, I really enjoy using Ktor.
I struggled to optimize "/update" endpoint though, maybe you know a better way 😅
Uh oh!
There was an error while loading. Please reload this page.