-
Notifications
You must be signed in to change notification settings - Fork 7
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
Store EditionsClientCard model for editions clipboard data #1584
Conversation
13a3f32
to
37a6f0c
Compare
Hmmmmmmmm. I see the logic of using I'm not scala expert though and would welcome an eye from someone who has more direct experience of shapeless etc. - @emdash-ie @JustinPinner what do you think? |
It might be worth adding some notes to the doc about the logic for the differences between |
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.
Does what it says on the tin 😀
I've left my 2p worth on the Either
/CoProduct
/something question in the comments... I don't think it's a blocker but I think it's worth a conversation with the rest of the team
I’m totally happy with this use of Either, and I don’t think using shapeless’ coproduct type would be a good idea: it’s far too awkward to work with for direct use. There’s nothing (beyond a dislike of boilerplate) stopping us from wrapping these in an extra ADT, which is how we can evolve this when we add a third type of thing: object ClipboardCard {
def apply(trail: Trail): ClipboardCard = TrailCard(trail)
def apply(editionsCard: EditionsClientCard): ClipboardCard = EditionsCard(editionsCard)
// presumably this could use TrailCard and EditionsCard directly, but I don’t know how offhand
val reads: Reads[ClipboardCard] =
JsPath.read[EditionsClientCard].map(ClipboardCard.apply) or
JsPath.read[Trail].map(ClipboardCard.apply)
// can we even get this and the Reads automatically once we’re using an ADT? Feels like it should be possible
val writes = new Writes[ClipboardCard] {
override def writes(o: ClipboardCard): JsValue =
o match {
case TrailCard(trail) => Json.toJson(trail),
case EditionsCard(editionsCard) => Json.toJson(editionsCard)
}
}
implicit val format = Format(reads, writes)
}
sealed trait ClipboardCard
case class TrailCard(card: Trail) extends ClipboardCard
case class EditionsCard(card: EditionsClientCard) extends ClipboardCard |
Thanks for the comments, all. @fredex42 yes, the third thing breaks this! @emdash-ie, that's a nice way of solving for that, and thanks for going to the trouble of writing out an example. I think in the back of my mind is the idea that I'd expect the number of entities to shrink, not grow, as we converge on a single representation of a trail/card across products, but this is a) a hunch and b) a hostage to fortune 😀 Sounds like we're happy enough to merge as-is, but if there's any additional feedback, let me know. |
👍 1524285 |
Seen on PROD (merged by @jonathonherbert 6 minutes and 23 seconds ago) Please check your changes! |
What's changed?
Store editions clipboard data as
EditionsClientCard
, notTrail
, which means thecardType
property will be persisted. At the moment, it's not appearing, and as a result recipe cards stored in the clipboard are blank.Implementation notes
The clipboard is the only place where the Fronts'
Trail
, and the Editions'EditionsClientCard
(both of which represent cards in those domains) coexist. So it was necessary to add a new model,ClipboardCard
, which can contain either of these models.Because
Trail
does not belong to thefacia-tool
project, it is not possible to create a sealed trait of both of these case classes, and because we are not running Scala 3, a union type is not possible. I've usedEither
, taking its 'this or that' semantic at face value, rather than resorting to adding a library like Shapeless and using something likeCoProduct
– I hope the intent is clear. We can circle back if there are strong opinions about this.Checklist
General
Client