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

Many to one #175

Merged
merged 15 commits into from
Mar 14, 2018
Merged

Many to one #175

merged 15 commits into from
Mar 14, 2018

Conversation

neilchaudhuri
Copy link
Contributor

Adds support for MultiEnum, which maps multiple entries to a single enum. Also uses breakOut for some minor memory optimization.

Copy link
Owner

@lloydmeta lloydmeta left a comment

Choose a reason for hiding this comment

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

The compilation failure is real; but otherwise looks ok to me :)

@@ -227,6 +227,19 @@ object UncapitalisedEnum extends Enum[UncapitalisedEnum] {

}

case object MultiEnum extends Enum[MultiEnum] {
import scala.collection.breakOut
val values = findValues
Copy link
Owner

Choose a reason for hiding this comment

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

You can't do this because the type parameter passed to Enum must be a sealed trait.

Compilation error https://travis-ci.org/lloydmeta/enumeratum/jobs/343456716#L844

[error] /home/travis/build/lloydmeta/enumeratum/enumeratum-core/src/test/scala/enumeratum/Models.scala:232:16: You can only use findValues on sealed traits or classes
[error]   val values = findValues
[error]                ^

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Fixed.

@@ -167,6 +175,9 @@ object EnumEntry {
*/
trait LowerCamelcase extends EnumEntry with Camelcase with Uncapitalised

abstract class MultiEnum(override val entryName: String,
Copy link
Owner

Choose a reason for hiding this comment

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

I don't think this is needed (at least not here); looks like something you want in your tests.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Fixed.

@@ -31,8 +31,8 @@ object EnumEntry {
*
* http://stackoverflow.com/a/19832063/1814775
*/
private val regexp1: Pattern = Pattern.compile("([A-Z]+)([A-Z][a-z])")
private val regexp2: Pattern = Pattern.compile("([a-z\\d])([A-Z])")
private val regexp1: Pattern = Pattern.compile("([A-Z]+)([A-Z][a-z])")
Copy link
Owner

Choose a reason for hiding this comment

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

Since we don't need the class you added, please revert these formatting changes.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Fixed.

lazy final val namesToValuesMap: Map[String, A] =
values.map(v => v.entryName -> v).toMap
lazy val namesToValuesMap: Map[String, A] =
values.map(v => v.entryName -> v)(breakOut)
Copy link
Owner

Choose a reason for hiding this comment

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

I'm not a fan of using breakOut here; I know what it does, but:

  1. It's not obvious to beginners how it works;
  2. It'll likely be gone with the new collections lib
  3. This is a cached value; the so gains are pretty minimal.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

tl;dr Removed breakOut.

breakOut is definitely complex since CanBuildFrom isn't exactly the most intuitive concept, and I know that its days are numbered in Scala collections--though it isn't deprecated and probably won't be for some time. It just won't be necessary with the new views.

Also, breakOut is a memory optimization to eliminate the need for an intermediate sequence--cached values or not. I worry that might matter when people use enums in "Big Data" (as with Apache Spark) scenarios, but it is definitely true that the sequences for most cases are so small that breakOut probably doesn't change the world.

Copy link
Owner

@lloydmeta lloydmeta left a comment

Choose a reason for hiding this comment

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

First of all thanks so much ! This is great work. Just one small suggestion and I think we should be good to go.

For sanity, we should update the other Maps, otherwise things can get weird.

@@ -38,7 +37,7 @@ trait Enum[A <: EnumEntry] {
/**
* Map of [[A]] object names to [[A]]s
*/
lazy final val namesToValuesMap: Map[String, A] =
lazy val namesToValuesMap: Map[String, A] =
Copy link
Owner

Choose a reason for hiding this comment

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

Nice.

I think we should probably modify these Maps to be in terms of namesToValuesMap as well (I'd like to keep them final for now)

lazy final val lowerCaseNamesToValuesMap: Map[String, A] =
values.map(v => v.entryName.toLowerCase -> v).toMap
/**
* Map of [[A]] object names in upper case to [[A]]s for case-insensitive comparison
*/
lazy final val upperCaseNameValuesToMap: Map[String, A] =
values.map(v => v.entryName.toUpperCase -> v).toMap

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Can you clarify what you mean here?

Copy link
Owner

Choose a reason for hiding this comment

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

Right now, they're built in terms of values directly, whereas I think they should be built in terms of namesToValuesMap so that they can have the same values (in case the user overrides namesToValuesMap and wants to use functions that call lowerCaseNamesToValuesMap)

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I think I got it.

@lloydmeta
Copy link
Owner

Actually, the tests are failing with valid failures,

See: https://travis-ci.org/lloydmeta/enumeratum/jobs/343869470#L2751


[info]   - should return the proper object when passed the proper string *** FAILED ***
[info]     java.util.NoSuchElementException: One is not a member of Enum (one, two)
[info]     at $c_ju_NoSuchElementException.$c_jl_Throwable.fillInStackTrace__jl_Throwable(/home/travis/build/lloydmeta/enumeratum/enumeratum-core/.js/target/scala-2.12/enumeratum-test-fastopt.js:27434:14)
[info]     at $c_ju_NoSuchElementException.$c_jl_Throwable.init___T__jl_Throwable(/home/travis/build/lloydmeta/enumeratum/enumeratum-core/.js/target/scala-2.12/enumeratum-test-fastopt.js:27466:8)
[info]     at java.util.NoSuchElementException.<init>(/home/travis/build/lloydmeta/enumeratum/enumeratum-core/.js/target/scala-2.12/enumeratum-test-fastopt.js:43347:52)
[info]     at enumeratum.Enum.withName(/home/travis/build/lloydmeta/enumeratum/enumeratum-core/.js/target/scala-2.12/enumeratum-test-fastopt.js:1193:46)
[info]     at {anonymous}()(/home/travis/build/lloydmeta/enumeratum/enumeratum-core/.js/target/scala-2.12/enumeratum-test-fastopt.js:75695:27)
[info]     at scala.scalajs.runtime.AnonFunction0.apply(/home/travis/build/lloydmeta/enumeratum/enumeratum-core/.js/target/scala-2.12/enumeratum-test-fastopt.js:36528:23)
[info]     at org.scalatest.OutcomeOf.outcomeOf(/home/travis/build/lloydmeta/enumeratum/enumeratum-core/.js/target/scala-2.12/enumeratum-test-fastopt.js:1852:7)
[info]     at org.scalatest.Transformer.apply(/home/travis/build/lloydmeta/enumeratum/enumeratum-core/.js/target/scala-2.12/enumeratum-test-fastopt.js:48096:10)
[info]     at org.scalatest.FunSpecLike$$anon$1.apply(/home/travis/build/lloydmeta/enumeratum/enumeratum-core/.js/target/scala-2.12/enumeratum-test-fastopt.js:38048:72)
[info]     at org.scalatest.FunSpecLike.invokeWithFixture$1(/home/travis/build/lloydmeta/enumeratum/enumeratum-core/.js/target/scala-2.12/enumeratum-test-fastopt.js:71598:15)

Pretty sure that's a fresh failure. Have you tried running the tests locally??

@neilchaudhuri
Copy link
Contributor Author

neilchaudhuri commented Feb 22, 2018

I ran test and got a message that all the tests passed. However, I didn't realize other tests failed to run because of an OutOfMemoryError. Meanwhile, the test in question failed because of bad setup. Sorry for the carelessness on both counts.

The tests should pass now.

@coveralls
Copy link

coveralls commented Feb 23, 2018

Coverage Status

Coverage remained the same at 100.0% when pulling 2c604d3 on neilchaudhuri:many-to-one into 6595742 on lloydmeta:master.

@neilchaudhuri
Copy link
Contributor Author

Everything should be in order except with regard to that one clarification I need. Can you elaborate on that? Thanks.

@lloydmeta
Copy link
Owner

@neilchaudhuri sorry for the radio silence I'm currently away on a business trip. Will take a closer look and answer any questions when I'm back !!

@neilchaudhuri
Copy link
Contributor Author

Oh OK. Good luck with the business and safe travels!

Copy link
Owner

@lloydmeta lloydmeta left a comment

Choose a reason for hiding this comment

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

LGTM!

@lloydmeta lloydmeta merged commit d328c23 into lloydmeta:master Mar 14, 2018
@lloydmeta
Copy link
Owner

I've just released this as part of 1.5.13 :)

@neilchaudhuri
Copy link
Contributor Author

Awesome. Thanks. I hope people find the feature useful.

@drewboardman
Copy link

Is there any documentation on how to use this? I'm not seeing MultiEnum available. Was this removed - or are we intended to also override nameToValuesMap?

@lloydmeta
Copy link
Owner

MultiEnum.withName("one") shouldBe MultiEnum.One
MultiEnum.withName("1") shouldBe MultiEnum.One
a[NoSuchElementException] should be thrownBy MultiEnum.withName("One")
MultiEnum.withName("eins") shouldBe MultiEnum.One
MultiEnum.withName("two") shouldBe MultiEnum.Two

^ should show usage?

@ChernikovP
Copy link

@drewboardman , apparently this MultiEnum is still and was always under test directory, rather than src, thus it's not actually available...

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

5 participants