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

Support pattern matching on NonEmptyVector #1265

Merged

Conversation

travisbrown
Copy link
Contributor

No description provided.

@kailuowang
Copy link
Contributor

👍

@codecov-io
Copy link

codecov-io commented Aug 4, 2016

Current coverage is 90.47% (diff: 100%)

Merging #1265 into master will not change coverage

@@             master      #1265   diff @@
==========================================
  Files           243        243          
  Lines          3286       3286          
  Methods        3231       3231          
  Messages          0          0          
  Branches         52         53     +1   
==========================================
  Hits           2973       2973          
  Misses          313        313          
  Partials          0          0          

Sunburst

Powered by Codecov. Last update 1612451...e7c093f

@@ -223,6 +223,8 @@ object NonEmptyVector extends NonEmptyVectorInstances {
new NonEmptyVector(buf.result)
}

def unapply[A](nev: NonEmptyVector[A]): Some[(A, Vector[A])] = Some((nev.head, nev.tail))
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Is there a foreseeable use-case where having the static return type as Some instead of Option will cause inference (or similar) issues?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@ceedubs There's some reason to prefer Some as the return type for extractors that can't fail—the compiler actually uses that information in some way, but I can't remember the details. I'll try to look up a reference…

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The compiler can check for exhaustiveness if it returns Some. It can't when it returns Option.

Copy link
Contributor

@dwijnand dwijnand Aug 4, 2016

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@larsrh Got a code snippet showing that?

AFAIK Some can't check exhaustiveness any more than Option as it's not sealed (sealedness isn't transitive nor does final imply sealed, double sadly).

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

scala> object MyCons1 { def unapply[A](xs: scala.collection.immutable.::[A]): Some[(A, List[A])] = Some(xs.head, xs.tail) }
defined object MyCons1

scala> object MyCons2 { def unapply[A](xs: scala.collection.immutable.::[A]): Option[(A, List[A])] = Some(xs.head, xs.tail) }
defined object MyCons2

scala> def x = List(1, 2) match { case MyCons1(_, _) => }
<console>:12: warning: match may not be exhaustive.
It would fail on the following input: Nil
       def x = List(1, 2) match { case MyCons1(_, _) => }
                   ^
x: Unit

scala> def x = List(1, 2) match { case MyCons2(_, _) => }
x: Unit

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

AFAICT it's unspecced behaviour, but I think it makes a lot of sense for the pattern matcher to behave that way.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Oh neat. Thanks, @larsrh!

@ceedubs
Copy link
Contributor

ceedubs commented Aug 4, 2016

👍

1 similar comment
@johnynek
Copy link
Contributor

johnynek commented Aug 4, 2016

👍

@kailuowang kailuowang merged commit b6d77fa into typelevel:master Aug 5, 2016
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.

7 participants