-
-
Notifications
You must be signed in to change notification settings - Fork 371
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
TodoMVC kotlin ktor #3651
TodoMVC kotlin ktor #3651
Conversation
|
||
> curl http://localhost:8080 | ||
...<h1>todos</h1>... | ||
|
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.
I think this can be fixed by de-denting all the lines in the usage block. The parser handling these is pretty fragile and doesn't tolerate extraneous whitespace
I'm seeing
|
As part of this PR, can you update 1-hello-ktor to use 8090 as its port, and 2-todo-ktor to user 8091. We currently don't have automatic port assignment set up, so we have to manually assign different ports to each integration test to avoid conflict when running in parallel |
One behavioral bug I found: when there are tasks stored, we should never hide the bottom All/Active/Completed bar, even if none of the tasks would be displayed in the current configuration. The bar should only be hidden when all tasks are deleted, not when they are hidden by filters |
Thanks for the review @lihaoyi. I'll take a look to these comments over the weekend :) |
Looks great, @javimartinez if you're done with it I'll merge once tests pass |
Yeah, I'm done :) |
Great! Email me your bank transfer details and I can send you the bounty |
Part of #3611