-
-
Notifications
You must be signed in to change notification settings - Fork 94
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
Add contramap
and map
ops to the Transformer
and PartialTransformer
#591
base: master
Are you sure you want to change the base?
Add contramap
and map
ops to the Transformer
and PartialTransformer
#591
Conversation
chimney-cats/src/main/scala/io/scalaland/chimney/cats/CatsPartialTransformerImplicits.scala
Show resolved
Hide resolved
chimney-cats/src/main/scala/io/scalaland/chimney/cats/CatsTotalTransformerImplicits.scala
Show resolved
Hide resolved
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.
Hello, thanks for another PR!
I think we need a parity of features where possible, so both Transformer
and PartialTransformer
should have both map
and contramap
if we're adding them.
Additionally, we would prefer these operations to not live in the main typeclass but more in some dedicated trait:
// docs
trait Transformer[From, To] extends AutoDerived[From, To] {
// docs
def transform(src: From): To
}
object Transformer {
trait AutoDerived[From, To] extends TransformerOps[From, To] {
def transform(src: From): To
}
trait TransformerOps[From, To] {
def transform(src: From): To
def map[A](f: To => A): Transformer[From, A] = ...
def contramap[A](f: A => From): Transformer[A, To] = ...
}
}
so that they could be called and be present in intellisense but not distract from the main use case of Transformer
s (and PartialTransformer
s) when looking in the source code.
* | ||
* @since 1.5.0 | ||
*/ | ||
final def map[A](f: To => A): PartialTransformer[From, A] = new PartialTransformer[From, A] { |
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.
If there is map
in partial, there could also be mapPartial[A](f: To => partial.Result[A]): PartialTransformer[From, A]
.
Since Transformer
has contramap
the PartialTransformer
should also get one.
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 thought about this once again and don't see any issue with adding contramap
to PartialTransformer
and map
to Transformer
, respectively.
What about mapPartial
, it might be useful indeed, but don't you mind if we would add it separately?
*/ | ||
final def contramap[A](f: A => From): Transformer[A, To] = new Transformer[A, To] { | ||
override def transform(src: A): To = self.transform(f(src)) | ||
} |
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.
Since PartialTransformer
has map
the Transformer
should also get one.
Transformer#contramap
and PartialTransformer#map
contramap
and map
ops to the Transformer
and PartialTransformer
Motivation
This PR adds four methods
Transformer#contramap
,Transformer#map
,PartialTransformer#contramap
andPartialTransformer#map
, to make transformers more composable and reusing existing transformers more concise and pleasant.Examples
Transformer#contramap
:PartialTransformer#map
:Why not use existing capabilities?
Transformer#contramap
users might doPartialTransformer#map
users might doWell, the main reason is to be concise and to promote reusing existing transformers (at the user's site), keeping the DRY principle at its max. These combinators are also widely known in functional communities, making their usage transparent and pleasant. OTOH, there is the
chimney-cats
module, which provides the same functionalities. However, using it would require many redundant allocations (implicit syntaxes come with their costs), which might not be ideal for GCs, especially since Chimney is often used in hot-paths of applications (based on my experience).I'd appreciate any feedback on that matter!