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

Drop case in Refined #160

Merged
merged 1 commit into from May 7, 2016
Merged

Drop case in Refined #160

merged 1 commit into from May 7, 2016

Conversation

dwijnand
Copy link
Contributor

@dwijnand dwijnand commented May 6, 2016

.. with before-and-after (newly created) spec changes

@dwijnand
Copy link
Contributor Author

dwijnand commented May 6, 2016

Btw, happy to iterate and tweak any minor detail, the intent of the change is my only goal.

@dwijnand
Copy link
Contributor Author

dwijnand commented May 6, 2016

Let me know what you want to do in terms of bincompat..

@fthomas
Copy link
Owner

fthomas commented May 6, 2016

MiMa complains about a lot of stuff that case brought in. Of those things, I think I only care about scala.Serializable. What do you think of the others?

@dwijnand
Copy link
Contributor Author

dwijnand commented May 6, 2016

I'm happy to recover Serializable.

The other one that's a shot-call on your part - do you want to provide Refined.unapply?

@fthomas
Copy link
Owner

fthomas commented May 7, 2016

Yes, Refined.unapply would be useful too.

@dwijnand
Copy link
Contributor Author

dwijnand commented May 7, 2016

Thanks to @ceedubs it seems this was already decided against in #57. Perhaps this alternative fix is more acceptable.

@dwijnand
Copy link
Contributor Author

dwijnand commented May 7, 2016

@fthomas Added Refined.unapply, squashed and pushed.

@fthomas
Copy link
Owner

fthomas commented May 7, 2016

Thanks @dwijnand! LGTM. So the only unsafe operations are now Refined.unsafeApply and asInstanceOf, right?

@fthomas fthomas merged commit 55fb5c0 into fthomas:master May 7, 2016
@dwijnand dwijnand deleted the non-case-class branch May 7, 2016 14:45
@dwijnand
Copy link
Contributor Author

dwijnand commented May 7, 2016

Refined.unsafeApply - yes. What do you mean by asInstanceOf? Can you show a code/repl snippet?

@fthomas
Copy link
Owner

fthomas commented May 7, 2016

scala> val x: Int Refined Positive = 5
x: eu.timepit.refined.api.Refined[Int,eu.timepit.refined.numeric.Positive] = 5

scala> x.asInstanceOf[Int Refined Negative]
res0: eu.timepit.refined.api.Refined[Int,eu.timepit.refined.numeric.Negative] = 5

@dwijnand
Copy link
Contributor Author

dwijnand commented May 7, 2016

Right.. yeah, that one too :)

@dwijnand
Copy link
Contributor Author

dwijnand commented May 7, 2016

Custom value classes don't have that problem.

@fthomas
Copy link
Owner

fthomas commented Oct 18, 2016

I'm wondering if we should make Refined a case class again by making it a sealed abstract case class. With this combination of keywords we don't get unsafe apply and copy methods.

@dwijnand
Copy link
Contributor Author

Can't have sealed value classes, as value classes are final.

@fthomas
Copy link
Owner

fthomas commented Oct 18, 2016

Good catch, @dwijnand. I've heard that the value class aspect of Refined was a reason for not using refined in the first place since value classes can't be nested. Maybe extends AnyVal should be dropped from Refined.

@dwijnand
Copy link
Contributor Author

Maybe value classes should be allowed to nest... :D

@fthomas
Copy link
Owner

fthomas commented Oct 18, 2016

I heard that is an easy change in scalac ;-)

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

Successfully merging this pull request may close these issues.

None yet

2 participants