coursera / courier

Data interchange for the modern web + mobile stack.
http://coursera.github.io/courier/
Apache License 2.0
98 stars 24 forks source link

Union unapply method does not disable match exhaustiveness checking #46

Closed marcgrr closed 7 years ago

marcgrr commented 7 years ago

The comment in the test describes what is going on.

Concretely, before this change, code like this would not cause scalac to output a non-exhaustive match warning, even though it's a non-exhaustive match:

def foo(a: WithComplexTypesUnion.Union) {
  println(a match {
    case WithComplexTypesUnion.Union.EmptyMember(x) => x
  })
}

After this change, that code will output a non-exhaustive match warning. (This applies to scala 2.11. It is possible that 2.12 does not suffer from the same problem, or will eventually not suffer from it.)

Unfortunately, this change introduces a small backwards-incompatibility for anyone compiling their scala code with -Xfatal-warnings: Code that used to compile won't necessarily still compile. I'm happy to fix up the Coursera code before incorporating this change, but are there any other consumers of this library that we have to think about?

amory-coursera commented 7 years ago

@marcgrr - Thanks for this change. I'm not aware of any non-Coursera users who use Courier with Scala (speak up if you're out there!). I think this change is worthy of a minor version bump (required to publish in any case...) but think it's an improvement worth making in spite of (possibly) breaking some existing code.

Can you run scripts/bumpVersion? Happy to merge in thereafter.

eboto commented 7 years ago

@amory-coursera us at Instrumental are non-Coursera Scala users of Courier. We have been wanting this feature for a while so this is fantastic. We have been preserving exhaustiveness checks by defaulting to pattern match against the type rather than against the unapplication. It will be great to stop needing that workaround.

Great addition @marcgrr !

amory-coursera commented 7 years ago

Thanks for letting us know, @eboto . From your commits, it seems like you folks at instrumental are polyglot. Super cool.

@marcgrr Please do bump the version and then merge.

marcgrr commented 7 years ago

Sorry for taking so long! I've bumped the version and I'll merge soon after the ci build finishes.

In the meantime, I've discovered a different similar issue: The record unapply method should probably also return Some. But there are some cases that I don't quite understand where it returns None, so I can't simply change the return type. I'll file an issue so that it doesn't get forgotten.