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

"Enum" classes are not implemented correctly #185

Closed
jpsacha opened this issue Mar 6, 2015 · 4 comments
Closed

"Enum" classes are not implemented correctly #185

jpsacha opened this issue Mar 6, 2015 · 4 comments
Labels
Milestone

Comments

@jpsacha
Copy link
Member

@jpsacha jpsacha commented Mar 6, 2015

Correct use of current "Enum" classes leads to compiler warnings due to incorrect implementation of those "Enum"s. Consider current implementation of class scalafx.geometry.Side:

object Side  extends SFXEnumDelegateCompanion[jfxg.Side, Side] {

  val BOTTOM = new Side(jfxg.Side.BOTTOM)
  val LEFT = new Side(jfxg.Side.LEFT)
  val RIGHT = new Side(jfxg.Side.RIGHT)
  val TOP = new Side(jfxg.Side.TOP)

  protected override def unsortedValues: Array[Side] = Array(TOP, BOTTOM, LEFT, RIGHT)
}

sealed case class Side(override val delegate: jfxg.Side) extends SFXEnumDelegate[jfxg.Side] {
  //...
}

If we match on an object of that class:

    val a: Side = ...

    a match {
      case Side.TOP =>
      case Side.BOTTOM =>
      case Side.RIGHT =>
      case Side.LEFT =>
    }

We will get compilation warning:

Warning:(49, 5) match may not be exhaustive.
It would fail on the following input: Side(_)
    a match {
    ^

This is despite the fact that we matched all possible values.

Correct pattern for implementing an "enum" is to use case objects for values and sealed abstract class like:

object Side extends SFXEnumDelegateCompanion[jfxg.Side, Side] {

  case object BOTTOM extends Side(jfxg.Side.BOTTOM)
  case object LEFT extends Side(jfxg.Side.LEFT)
  case object RIGHT extends Side(jfxg.Side.RIGHT)
  case object TOP extends Side(jfxg.Side.TOP)

  protected override def unsortedValues: Array[Side] = Array(TOP, BOTTOM, LEFT, RIGHT)
}

sealed abstract class Side(override val delegate: jfxg.Side) extends SFXEnumDelegate[jfxg.Side] {
  // ...
}

Now there will be no compilation warnings if we do a full match, and proper warning if we miss a case.

All "enum" in ScalaFX need to be impemented using this pattern.

@jpsacha jpsacha added the bug label Mar 6, 2015
@jpsacha

This comment has been minimized.

Copy link
Member Author

@jpsacha jpsacha commented Mar 6, 2015

It may be possible to get rid of the need for explicit declaration of unsortedValues using solution similar to one used by Enumeratum.

@Facsimiler

This comment has been minimized.

Copy link
Member

@Facsimiler Facsimiler commented Mar 6, 2015

Can't recall the bug number, but don't forget that Scala constants should be camel case with a leading capital. Maybe we should fix both at the same time.

Incidentally how would we efficiently implement enums that are just named integers in Java? If we use case objects, then we can't extend an AnyVal abstract base class (I think).

Also, regardless of the type stored, this pattern requires an implicit conversion function to convert the case object to its value.

@Facsimiler

This comment has been minimized.

Copy link
Member

@Facsimiler Facsimiler commented Mar 6, 2015

The other issue is #75.

@jpsacha

This comment has been minimized.

Copy link
Member Author

@jpsacha jpsacha commented Mar 11, 2015

The example in the description was copied from the source code that is still using Java naming for constants. Issue #75 needs to fixed too.

@jpsacha jpsacha added help wanted and removed help wanted labels Oct 15, 2017
@jpsacha jpsacha added this to the 8.0.*-R14 milestone Sep 30, 2018
jpsacha added a commit that referenced this issue Oct 12, 2018
@jpsacha jpsacha closed this Oct 12, 2018
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
2 participants
You can’t perform that action at this time.