-
Notifications
You must be signed in to change notification settings - Fork 219
Bump scalatest, make scala 3 compatible (Ready for review) #279
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
Conversation
r.hmget("hash", "field1", "field2") should be(Some(Map("field1" -> "1", "field2" -> "2"))) | ||
|
||
implicit val parseInt = Parse[Int](new String(_).toInt) | ||
{ |
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.
without these brackets, the earlier call to r.hmset will error in scala 3.0.2 with a message about an incorrect forwards-reference to parseInt
r.hmget("hash1", "field1", "field2") should be(Some(Map("field1" -> "Upper(val1)", "field2" -> "Upper(val2)"))) | ||
r.hmget("hash2", "field1", "field2") should be(Some(Map("field1" -> "VAL1", "field2" -> "VAL2"))) | ||
|
||
{ |
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.
ditto
implicit val parseLong = Parse[Long](new String(_).toLong) | ||
r.sortNStore[Long]("alltest", storeAt = "skey").getOrElse(-1) should equal(4) | ||
r.lrange("skey", 0, 10).get should equal(List(Some(1), Some(3), Some(10), Some(30))) | ||
{ |
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.
ditto
@hughsimpson whisklabs/docker-it-scala#146 is still Open - any idea when it will be merged ? |
Not sure if it'll ever be merged 😓 that repo seems extremely quiet - doesn't seem like any contributions have been merged since 23 Jun 2019. Might be necessary to publish a fork... EDIT: I emailed someone at whisk, they should be looking at the repo in the next week or so and figuring out the maintainance team, so might be soon 🤞 Edit x2 (11/11): I'm now being told it'll be another week. But they're updating me, so it's on their radar at least @debasishg That pr has now been merged and released, so this should be ready for review |
61f627f
to
6c9fbb0
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.
LGTM
@hughsimpson Can u please take a look at the failed github action ? https://github.com/debasishg/scala-redis/runs/4505252759?check_suite_focus=true. The |
Yep it's gone wonky. Not sure what happened there, I'll double-check my end but I'm fairly sure it must've been a shonky release with the docker-testkit-impl-docker-java dependency. EDIT: Actually, looks like some gigantic pr https://github.com/whisklabs/docker-it-scala/pull/149/files that made massive changes to the API got merged before the releases were cut, so this is probably something we can recover from. It's late for me today, but I'll check this out first thing tomorrow and try to get a fix in. I should've double-checked... Didn't realise they'd made so many additional changes, or I wouldn't have flagged this pr as ready-for-review 🤦 |
The main change here is bumping scalatest, however there are a few other small changes. The common theme is that, once whisklabs/docker-it-scala#146 (or similar version bump pr) is published, the project will build with scala 3Cross-compile with scala 3 should now work with artifacts available in maven central. The docker-testkit-impl-docker-java dependency is a beta, but since it's just a test dependency I don't think that should be a blocker