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

Provide withName* variants returning an Either[String, EnumEntry] #258

Merged
merged 4 commits into from
Dec 31, 2019
Merged

Provide withName* variants returning an Either[String, EnumEntry] #258

merged 4 commits into from
Dec 31, 2019

Conversation

pdalpra
Copy link
Contributor

@pdalpra pdalpra commented Oct 31, 2019

Similar to the Option variants, this provide safe functions to get an EnumEntry by name, returning a Left is the entry cannot be found.
This reuses the helper function to builds the error message for Exception.

NB: this can eventually be reused in some integrations (Circe, Play) that uses Either in their APIs, which currently relies on the withName* Option variants and pattern matches on the result to convert it to Either.

@coveralls
Copy link

coveralls commented Nov 1, 2019

Coverage Status

Coverage increased (+0.1%) to 90.707% when pulling 06ad65a on pdalpra:enum-withname-either into ed45b0e on lloydmeta:master.

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.

Wow, good idea. Would be nice if you could add this to the ValueEnum variants as well.

@pdalpra
Copy link
Contributor Author

pdalpra commented Nov 1, 2019

Indeed, I'll add right away !
Regarding the build failure, it seems that there is a transient issue on some Scala versions, but not always the same version 🤔

@pdalpra
Copy link
Contributor Author

pdalpra commented Nov 10, 2019

Is is okay for you ?

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.

This LGTM, but I'm away at the moment so will hold off on merging.

Wondering if String is a good type to return in the left or we should create a newtype wrapper around it (e.g. final case class NoSuchEntry(message: String) extends AnyVal) to make it more type safe...

@Qthunder
Copy link

Qthunder commented Dec 6, 2019

Was just about implement this very same feature :) Let me know if I can help

@pdalpra
Copy link
Contributor Author

pdalpra commented Dec 7, 2019

I didn't get time recently to work on it, hopefully this week end :)

@pdalpra
Copy link
Contributor Author

pdalpra commented Dec 28, 2019

Sorry for the delay, finally got time to update this PR, with the change discussed above:

  • Introduce NoSuchMember case classes (one for Enum, one for ValueEnum)
  • Extend NoSuchElementException and NoStackTrace
  • Override getMessage instead of using exceptions constructors taking a constructor to not allocate the string if not needed

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.

Looking good, left a couple of suggestions for improvement.

@@ -85,6 +85,13 @@ trait Enum[A <: EnumEntry] {
*/
def withNameOption(name: String): Option[A] = namesToValuesMap.get(name)

/**
* Returns an [[Right[A]]] for a given name, or a [[Left[String]]] if the name does not match any of the values'
Copy link
Owner

Choose a reason for hiding this comment

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

Suggested change
* Returns an [[Right[A]]] for a given name, or a [[Left[String]]] if the name does not match any of the values'
* Returns an [[Right[A]]] for a given name, or a [[Left[NoSuchMember]]] if the name does not match any of the values'

enumeratum-core/src/main/scala/enumeratum/Enum.scala Outdated Show resolved Hide resolved
* Returns an [[Right[A]]] for a given name, or a [[Left[String]]] if the name does not match any of the values'
* .entryName values, disregarding case.
*/
def withNameUppercaseOnlyEither(name: String): Either[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.

Same here.

* Returns an [[Right[A]]] for a given name, or a [[Left[String]]] if the name does not match any of the values'
* .entryName values, disregarding case.
*/
def withNameLowercaseOnlyEither(name: String): Either[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.

Same here

@pdalpra
Copy link
Contributor Author

pdalpra commented Dec 29, 2019

All fixed, completely missed the documentation and case variants methods 😅

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. Will merge and release in the next couple days :)

@lloydmeta lloydmeta merged commit 1188ca5 into lloydmeta:master Dec 31, 2019
@lloydmeta
Copy link
Owner

Hey, thanks for this @pdalpra . Just released 1.5.15 based on this.

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

4 participants