-
Notifications
You must be signed in to change notification settings - Fork 120
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
Cross-compile for Scala 3 #561
Conversation
@@ -50,7 +50,7 @@ trait CacheKeySpecCommon extends Suite with Matchers with BeforeAndAfter { | |||
|
|||
} | |||
|
|||
class AClass[F[_]](implicit cache: Cache[F, Int]) { | |||
class AClass[F[_]]()(implicit cache: Cache[F, Int]) { |
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.
Added the extra ()
here just so they're included in the scala 3 generated cache keys (otherwise CacheKeyIncludingConstructorParamsSpec."work for a method inside a class"
fails as it would generate "scalacache.memoization.AClass(${cache.toString}).insideClass(1)"
rather than "scalacache.memoization.AClass()(${cache.toString}).insideClass(1)"
)
For compatibility on this, it should be enough to check if the first element of the macro call to .paramss is implicit, and prefix an empty list in that case, but it feels rather OTT, especially assuming that most usages of this lib will be using a local, rather than a distributed, cache
@@ -33,7 +33,7 @@ object MethodCallToStringConverter { | |||
|
|||
private def appendClassNamePart(sb: JStringBuilder)(className: String): Unit = { | |||
if (className.nonEmpty) { | |||
sb.append(className) | |||
sb.append(className.stripSuffix("$")) |
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.
Ugly... Just to retain the exact same format for keys generated for the tests. Won't even do that in general, since an object Foo { object Bar { def baz = memoize { ... } }}
would have Foo$.Bar.baz
in scala 3 rather than Foo.Bar.baz
- so might be worth figuring out how to rewrite the test assertions appropriately to acknowledge the slightly different className formats between scala 2 and scala 3...
Of course a String.replace("$.", ".") at the end would permit full compatibility on that front, but would be more expensive... I don't know whether full compatibility of keys between Scala versions (useful, I guess, for Redis) is a goal of this project, so I've avoided it here
@@ -42,7 +42,7 @@ object MethodCallToStringConverter { | |||
sb: JStringBuilder | |||
)(className: String, constructorParamss: IndexedSeq[IndexedSeq[Any]]): Unit = { | |||
if (className.nonEmpty) { | |||
sb.append(className) | |||
sb.append(className.stripSuffix("$")) |
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 (see comment on the other stripSuffix
)
@@ -99,7 +99,7 @@ class Bar(a: Int) { | |||
then you want the cache key to depend on the values of both `a` and `b`. In that case, you need to use a different implementation of [MethodCallToStringConverter](https://github.com/cb372/scalacache/blob/master/modules/core/shared/src/main/scala/scalacache/memoization/MethodCallToStringConverter.scala), like this: | |||
|
|||
```scala mdoc:silent | |||
implicit val cacheConfig = CacheConfig( | |||
implicit lazy val cacheConfig: CacheConfig = CacheConfig( |
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.
Seems like the implicit val catsCache: Cache[IO, Cat] = MemcachedCache("localhost:11211")
earlier on in this file constructs a forward reference to this implicit val (presumably it didn't before the tut->mdoc change); so this needs to be lazy or there's a weird NPE/AbstractMethodError (depending on scala version) thrown when running docs/mdoc
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.
Looks like the rest works without this snippet, so I'd rather remove the lazy but exclude this snippet from mdoc.
val sym: Symbol = Symbol.spliceOwner | ||
|
||
def hasCacheKeyExcludeAnnotation(s: Symbol): Boolean = s.annotations.exists { | ||
// case Apply(Select(New(Ident("cacheKeyExclude")), _), _) => true // Don't know why this doesn't match... |
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.
funny, summon[TypeTest[Tree, Ident]].unapply(o)
gives me a None too. Guess that's what's used, since Ident
is a type member, thus prone to type erasure, so TypeTest
will be used underneath (like ClassTag
would be used in scala 2).
No idea why it doesn't match though. Maybe it's worth reporting somewhere...
@@ -99,7 +99,7 @@ class Bar(a: Int) { | |||
then you want the cache key to depend on the values of both `a` and `b`. In that case, you need to use a different implementation of [MethodCallToStringConverter](https://github.com/cb372/scalacache/blob/master/modules/core/shared/src/main/scala/scalacache/memoization/MethodCallToStringConverter.scala), like this: | |||
|
|||
```scala mdoc:silent | |||
implicit val cacheConfig = CacheConfig( | |||
implicit lazy val cacheConfig: CacheConfig = CacheConfig( |
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.
Looks like the rest works without this snippet, so I'd rather remove the lazy but exclude this snippet from mdoc.
This is good, thanks :) |
Introduced a couple of little warts to retain identical keys for the tests (to avoid having to refactor them too much). Added comments on those locations.
Most of the changes are just formatting from bumping the scalafmt version...
Also (think I've) fixed the docs/mdoc call now 🤞 seems to work locally
Hopefully this is useful to someone else