shekhargulati / 52-technologies-in-2016

Let's learn a new technology every week. A new technology blog every Sunday in 2016.
https://shekhargulati.com/
MIT License
7.19k stars 598 forks source link

Feedback: Week 4 Slick #6

Open shekhargulati opened 8 years ago

shekhargulati commented 8 years ago

Please provide your feedback by posting a comment against this issue.

llcawthorne commented 8 years ago

I'm trying to follow along, but it can't seem to find my implicit conversions. I checked out the github repo, and get the same errors when trying sbt compile against your copy:

[error] /Users/Lewis/Downloads/BookCode/52-technologies-in-2016/04-slick/tasky/src/main/scala/datamodel/DataModel.scala:25: could not find implicit value for parameter tt: slick.ast.TypedType[java.time.LocalDateTime]
[error]     def createdAt = column[LocalDateTime]("createdAt")
[error]                                          ^
[error] /Users/Lewis/Downloads/BookCode/52-technologies-in-2016/04-slick/tasky/src/main/scala/datamodel/DataModel.scala:27: could not find implicit value for parameter tt: slick.ast.TypedType[java.time.LocalDateTime]
[error]     def dueBy = column[LocalDateTime]("dueBy")
[error]                                      ^
[error] /Users/Lewis/Downloads/BookCode/52-technologies-in-2016/04-slick/tasky/src/main/scala/datamodel/DataModel.scala:29: could not find implicit value for parameter tt: slick.ast.TypedType[Set[String]]
[error]     def tags = column[Set[String]]("tags")
[error]                                   ^
[error] three errors found
[error] (compile:compileIncremental) Compilation failed
[error] Total time: 3 s, completed 14-Feb-2016 15:01:43

I can compile Part 5 from the github repo, so it must be different somehow there. I'll see what's up with it when I get to it, but for now, I just got rid of the implicit conversions and used Timestamp and String data types for the fields of my object.

I'm not really sure of a difference in section 5, other than the object being in its on scala file instead of further down in the same file... It worked great when I split them off to the separate file and imported them instead of having them in a different object within the same file but including an import statement anyway, which is how it is here. Creating ColumnDataMappers like in part 5 and priority, then updating my tests to use LocalDateTime instead of manually doing timestamps got me back on track and working quick enough.

llcawthorne commented 8 years ago

Small issue. The blogpost part doesn't describe the beforeEach method that you've implemented in the code example to recreate the database for each test. As it is written, following along leads to having four entries in the database when you run the test to insert 3 items. If you try to re-run it, then the database isn't created yet.

Seems easy enough to fix by looking at the code example with beforeEach/afterEach, but even with the afterEach doing a db.shutdown I am getting an error about tasks already existing when it tries to create the table that causes it not to run any test after the first... Maybe it will make more sense when I read the next blogpost.

I worked around it by adding a def dropTaskTableAction = Tasks.schema.drop that I can't imagine really wanting in a production model and just calling it instead of db.shutdown in the afterEach.

BTW, thanks so much for writing these. I'm finding them all quite interesting!

shekhargulati commented 8 years ago

@llcawthorne thanks for the feedback. Below I have replied to your comments.

  1. The code was not compiling because I had to explicitly set the column mapper in the column definition https://github.com/shekhargulati/52-technologies-in-2016/blob/master/04-slick/tasky/src/main/scala/datamodel/DataModel.scala#L25. I have pushed the changes so code should now compile fine and tests should pass.
  2. I will add section on beforeEach. I think the reason db.shutdown was not working for you is that db.shutdown runs asynchronously so I have to wrap that in Await.result as well. It was working on my machine so I didn't noticed that behaviour. I have pushed the changes. Please try and let me know if you still face so issue.

Also, I would be happy to accept any pull request that helps to improve the content.