Skip to content
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

Move FUUIDVar#unapply into FUUID? #70

Closed
gabro opened this issue Nov 21, 2018 · 5 comments
Closed

Move FUUIDVar#unapply into FUUID? #70

gabro opened this issue Nov 21, 2018 · 5 comments

Comments

@gabro
Copy link
Contributor

gabro commented Nov 21, 2018

FUUIDVar from the http4s module simply defines an unapply method, which is "standard" and not specific to http4s in any way.

Could it make sense to move that unapply into the core FUUID object so that one would get path extractors out of the box without the extra dependency?

import io.chrisdavenport.fuuid.FUUID

Path("/v1/16a30838-5e02-4e68-a1bc-585e42594430") match {
  case Root / "v1" / FUUID(uuid) => // do something with uuid
  case _ => // nope
}

I understand that conceptually the unapply method is there to support http4s extractors, but it could technically be useful in any other match clause regardless of https.
This is a result of the cool design of http4s' DSL, and it would be interesting to exploit it :)

The http4s module is still useful for http4s-specific things like #69

@gabro gabro changed the title Move FUUIDVar into core? Move FUUIDVar#unapply into FUUID? Nov 21, 2018
@ChristopherDavenport
Copy link
Collaborator

The type having an unapply that exposes itself internally seems odd, and exposing something that interacts with the underlying is unsafe in a way I would prefer not to expose.

How do you see this being used outside of http4s dsl?

@gabro
Copy link
Contributor Author

gabro commented Jan 16, 2019

One example I have from a project of mine goes a follows:

import io.estatico.newtype.macros.newtype
import io.chrisdavenport.fuuid.FUUID
import io.chrisdavenport.fuuid.http4s.FUUIDVar

@newtype case class UserId(value: FUUID)
object UserId {
  def unapply(str: String): Option[UserId] = FUUIDVar.unapply(str).map(UserId(_))
}

And I will then use UserId.unapply in pattern matches outside of the http4s DSL.

In this case, I had to bring in FUUIDVar from the http4s package even though I'm not using http4s at all.

Anyway, this is just an example, and I'm not particularly attached to the proposal. It just seemed strange to be forced to depend on the http4s package for something (unapply) that is potentially unrelated.

@ChristopherDavenport
Copy link
Collaborator

I would think using the core types would be more direct. Since it doesn't go through the indirection. We could probably put a helper in to do the option conversion.

https://github.com/ChristopherDavenport/fuuid/blob/series/0.2/modules/core/src/main/scala/io/chrisdavenport/fuuid/FUUID.scala#L36

def unapply(str: String): Option[UserId] = 
  FUUID.fromString(str).map(UserId(_)).toOption

@ChristopherDavenport
Copy link
Collaborator

If using #85 - this could be FUUID.fromStringOpt(str).map(UserId(_))

I guess I'm not seeing what the unapply is exposing. As its not part of a larger hierarchy or any other system that it could be yanked out of generally.

However I basically only use unapply for the http4s dsl use.

@gabro
Copy link
Contributor Author

gabro commented Jan 16, 2019

Yeah, a fromStringOpt method is definitely a good solution. Thanks for the feedback and for the API addition 👍

@gabro gabro closed this as completed Jan 16, 2019
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

No branches or pull requests

2 participants