-
Notifications
You must be signed in to change notification settings - Fork 5
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
Rewrite the todolist example to remove the frees dependency #4: Part 2 -- the rest of the changes #12
Conversation
todolist/server/src/main/scala/handlers/TagRpcServiceHandler.scala
Outdated
Show resolved
Hide resolved
…of a companion object for a tagless trait here: https://github.com/higherkindness/mu-scala/pull/296/files\#diff-5b4d519858d4e95d347b19d70fa933edR49
Hmmm going to leave this in WIP mode because even though all the checks are green I got an IOException for the DB connection when running it locally; which I'll look into after lunch. I think I wrote one of my queries wrong. |
…I need is the companion object
Woohoo the app runs and the DB connection has been refactored! I'm promoting this PR from "draft" status to "ready for review" :) |
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.
Left some comments but overall the PR looks good to me
todolist/persistence/src/main/scala/runtime/TodoListRepositoryHandler.scala
Outdated
Show resolved
Hide resolved
todolist/persistence/src/main/scala/runtime/TagRepositoryHandler.scala
Outdated
Show resolved
Hide resolved
todolist/persistence/src/main/scala/runtime/queries/TodoItemQueries.scala
Outdated
Show resolved
Hide resolved
todolist/server/src/main/scala/handlers/TodoItemRpcServiceHandler.scala
Outdated
Show resolved
Hide resolved
thanks for the review, @fedefernandez! I was out on Friday for the US holiday last week so I'll address your comments today :) |
9f0a008
to
6223cd0
Compare
I think this is probably fine but it's a bit hard to review with your other branch merged into it. Maybe merge your other PR first, then rebase this one against master? |
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 is probably fine but it's a bit hard to review with your other branch merged into it. Maybe merge your other PR first, then rebase this one against master?
Yeah I agree and sorry for the headache. Just rebased; the PR should look cleaner now.
Closes #4