Unrecognised enum's constructor could be private

[matepek]

Hello,
I'm relatively new to this repo so forgive me if my lack of experience results in wasting your time.

Using the Weather example:

    // In ScalaPB >= 0.5.x, this captures unknown value that are received
    // from the wire format.  Earlier versions throw a MatchError when
    // this happens.
    case class Unrecognized(value: Int) extends Weather { ... }

This results that:

assert(Weather.SUNNY == Weather.fromValue(1))
assert(Weather.SUNNY != Weather.Unrecognized(1)) // That's not what some might would expect.

The problem is the second one I believe.

Why?:
I almost wrote something like that but then I realized that it wouldn't work so I got curious that others made the same mistake and it turned out: yes.

How?:
As I can see/guess the enum was part of the dependency library but they were in a hurry and just added the not existing enum

x == Foo.Unrecognised(42) // TODO: update enum list

This worked at the time it was written. But someone added the enum to the dependency library and published and then later someone updated the version which contains the new enum. That's when things went poo poo.

The x == Foo.fromValue(42) should be fine but mistakes happen.

AFAIK this could be prevented by making Unrecognised private. (For backward compatibility it can be even an option in the config.) (private final case class Unrecognised(...)

What do you think?

[thesamet]

Hi @markosski , thanks for reporting. For messages, we have an annotations option that allow users to add a private. I will be happy to review a PR that adds support for annotating enum cases and Unrecognized. The options should be at the file-level (so they can apply globally via package-scoped option), and at the enum-level.

[matepek]

Annotations would work but a separate option gives more visibility to this feature, don't you think?

[matepek]

That's the idea. What do you think? Shall I start writing tests?