Skip to content

refactor: change Message kind into an enum instead of a String #14791

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

Merged
merged 1 commit into from
Mar 27, 2022

Conversation

ckipp01
Copy link
Member

@ckipp01 ckipp01 commented Mar 27, 2022

While digging into Diagnostics there wasn't an easy way to tell all the
available kinds that there could be since they were just a string
value. So this small changes just makes the kind field in Message a
MessageKind which is an enum of all the types of message kinds that
exist.

@ckipp01 ckipp01 force-pushed the messageKind branch 2 times, most recently from 7c76421 to abd0b2a Compare March 27, 2022 10:13
@som-snytt
Copy link
Contributor

som-snytt commented Mar 27, 2022

Alternative to relying on toString everywhere

enum Thing:
  def message: String =
    this match
    case Other => "some other thing"
    case x => s"Thing $x"

  case One, Two, Other

@main def test() = println {
  Thing.values.to(List).map(_.message)
}

(Trying to recall if that is my first Scala 3 enum.)

@ckipp01
Copy link
Member Author

ckipp01 commented Mar 27, 2022

Alternative to relying on toString everywhere

enum Thing:
  def message: String =
    this match
    case Other => "some other thing"
    case x => s"Thing $x"

  case One, Two, Other

@main def test() = println {
  Thing.values.to(List).map(_.message)
}

(Trying to recall if that is my first Scala 3 enum.)

The thing I'm afraid for with this though is that somewhere someone will just rely on the toString because they are used to it with enums. I sort of felt like this might be clearer. I think I'd rather do `Some Other Thing` first almost. Wdyt?

@som-snytt
Copy link
Contributor

Oh, you mean they're used to "kind" being "just a string"? I would not sweat that. But I haven't looked closely at the interpolators for messaging. I hope they don't just toString. I saw Dale's "use show" PR but haven't looked twice yet. Maybe this is the moment when, as smarter would say, it becomes "principled". Others would say "reasonable".

@ckipp01
Copy link
Member Author

ckipp01 commented Mar 27, 2022

Alright, I took your suggestion @som-snytt. Thanks!

While digging into Diagnostics there wasn't an easy way to tell all the
available `kind`s that there could be since they were just a string
value. So this small changes just makes the `kind` field in `Message` a
`MessageKind` which is an enum of all the types of message kinds that
exist.
@smarter smarter enabled auto-merge March 27, 2022 21:31
@smarter smarter merged commit 4848748 into scala:main Mar 27, 2022
@ckipp01 ckipp01 deleted the messageKind branch March 28, 2022 05:46
@Kordyjan Kordyjan added this to the 3.1.3 milestone Aug 1, 2023
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.

4 participants