Currency instances #4564
Currency instances #4564
Conversation
kernel/src/main/scala/cats/kernel/instances/CurrencyInstances.scala
Outdated
Show resolved
Hide resolved
| implicit private val arbitraryCurrency: Arbitrary[Currency] = Arbitrary( | ||
| Gen.oneOf(Currency.getAvailableCurrencies().asScala) | ||
| ) | ||
| implicit private val cogenCurrency: Cogen[Currency] = Cogen[String].contramap(_.getCurrencyCode()) |
There was a problem hiding this comment.
Perhaps, it would not hurt if we put these instances into cats.laws.discipline.arbitrary to make them available for everyone.
There was a problem hiding this comment.
I'm wrapping my head around the dependency graph here. Doesn't the laws module depend on this one? If we want to add these instances to kernel-laws, is there a way to add jvm-only code without moving too many files (kernel-laws is defined as CrossType.Pure)? Sorry not very used to cross compilation.
There was a problem hiding this comment.
Yeah, it is quite confusing. If it is not possible to do it in a simple way, let's keep it private in the tests for now then.
| @@ -48,4 +49,8 @@ private[cats] object scalaVersionSpecific { | |||
| that: T | |||
| )(implicit w1: A => TraversableLike[El1, Repr1], w2: T => IterableLike[El2, Repr2]) = (a, that).zipped | |||
| } | |||
|
|
|||
| implicit class setExtension[A](private val a: A) extends AnyVal { | |||
| def asScala(s: java.util.Set[A]): mutable.Set[A] = `set asScala`(s) | |||
There was a problem hiding this comment.
@armanbilge any better idea?
|
Is the |
|
Hi @massimosiani , sorry for the long reply. Looks like your PR got buried under a pile of others, although it looks pretty mature. Would you be interested to move it forward? In such a case, would you update it to the current |
|
Hi @satorg, of course! Thanks for the update, I'll do it ASAP. |
massimosiani
force-pushed
the
currency-instances
branch
from
3c5f94f to
58de8f0
Compare
| @@ -21,7 +21,8 @@ | |||
|
|
|||
| package cats.kernel.compat | |||
| import scala.annotation.{Annotation, StaticAnnotation} | |||
| import scala.collection.{IterableLike, TraversableLike} | |||
| import scala.collection.{mutable, IterableLike, TraversableLike} | |||
| import scala.collection.convert.ImplicitConversionsToScala.* | |||
There was a problem hiding this comment.
| import scala.collection.convert.ImplicitConversionsToScala.* | |
| import scala.collection.JavaConverters._ |
then you can call a conversion method explicitly, e.g.: asScalaSet(s), asScalaWhatever(x), etc.
Also please note that Cats hasn't switched to Scala3 import style yet, see discussion in #4677.
| implicit class setExtension[A](private val s: java.util.Set[A]) extends AnyVal { | ||
| def asScala: mutable.Set[A] = `set asScala`(s) | ||
| } |
There was a problem hiding this comment.
This extension method is only used for Gen.oneOf in tests, and the latter only requires Iterable. So I'd suggest to go with the minimally required type here as well:
| implicit class setExtension[A](private val s: java.util.Set[A]) extends AnyVal { | |
| def asScala: mutable.Set[A] = `set asScala`(s) | |
| } | |
| implicit class iterableExtension[A](private val s: java.lang.Iterable[A]) extends AnyVal { | |
| def asScala: Iterable[A] = iterableAsScalaIterable(s) // assuming import scala.collection.JavaConverters._ | |
| } |
There was a problem hiding this comment.
Besides, if this extension method is for the sake of testing only, I wonder if it'd make sense to move it from "main" to "test" scope?
| @@ -35,4 +37,8 @@ private[cats] object scalaVersionSpecific { | |||
| implicit class iterableOnceExtension[A](private val io: IterableOnce[A]) extends AnyVal { | |||
| def reduceOption(f: (A, A) => A): Option[A] = io.iterator.reduceOption(f) | |||
| } | |||
|
|
|||
| implicit class setExtension[A](private val s: java.util.Set[A]) extends AnyVal { | |||
| def asScala: mutable.Set[A] = SetHasAsScala(s).asScala | |||
There was a problem hiding this comment.
I doubt that SetHasAsScala is a part of any API. Not mention this call can allocate a class instance without a reason. I'd probably give a shot to scala.jdk.javaapi.CollectionConverters here, e.g.:
import scala.jdk.javaapi.CollectionConverters
...
def asScala: Iterable[A] = CollectionConverters.asScala(x)
| @@ -35,4 +36,8 @@ private[cats] object scalaVersionSpecific { | |||
| implicit class iterableOnceExtension[A](private val io: IterableOnce[A]) extends AnyVal { | |||
| def reduceOption(f: (A, A) => A): Option[A] = io.iterator.reduceOption(f) | |||
| } | |||
|
|
|||
| implicit class setExtension[A](private val s: java.util.Set[A]) extends AnyVal { | |||
| def asScala: Iterable[A] = CollectionConverters.asScala(x) | |||
There was a problem hiding this comment.
| def asScala: Iterable[A] = CollectionConverters.asScala(x) | |
| def asScala: Iterable[A] = CollectionConverters.asScala(s) |
massimosiani
force-pushed
the
currency-instances
branch
from
e38b59e to
15861a7
Compare
massimosiani
force-pushed
the
currency-instances
branch
from
15861a7 to
1ceeb9a
Compare
Addresses #4562.
I followed the steps outlined here #3910 (comment), there's no
Currencyin scala js/native tough.