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

Consider replacing all enum types with classes containing static constants #645

Open
KamasamaK opened this issue Jul 7, 2022 · 2 comments

Comments

@KamasamaK
Copy link
Contributor

The convention we currently use in LSP4J is to have enum types for the integer-based enumerations and classes containing static constants for the string-based enumerations.

I propose we make them all classes containing static constants. There are three reasons I can express for this change.

  1. Consistency.
  2. We can define the classes containing static constants in the Protocol.xtend instead of having to create separate Java files for each enum.
  3. It complies with a recommendation in the spec. In the spec explanation for enumerations, it says that for unknown enumerations the using side of an enumeration "should simply ignore it as a value it can use and try to do its best to preserve the value on round trips." Currently, an unknown enum would become null and thus not be preserved.

This would be a breaking change.

@pisv
Copy link
Contributor

pisv commented Jul 7, 2022

Also, returning null in case of an unknown value for enumerations is problematic for required (@NonNull) properties, like NotebookCell.kind.

So, the limitations of using Java enum classes for modeling LSP enumerations are apparent.

However, could not we try to come up with an alternative approach that would still be type-safe and object-oriented, e.g. by generating wrapper classes for enumerations with Xtend? It might then be used consistently for both integer- and string-based enumerations.

@pisv
Copy link
Contributor

pisv commented Jul 7, 2022

Just as a sketch, what if this declaration in Xtend:

/**
 * A notebook cell kind.
 * <p>
 * Since 3.17.0
 */
@JsonRpcEnum
class NotebookCellKind {
    /**
     * A markup-cell is formatted source that is used for display.
     */
    public static val Markup = 1;

    /**
     * A code-cell is source code.
     */
    public static val Code = 2;
}

would have been translated to the following class in Java:

/**
 * A notebook cell kind.
 * <p>
 * Since 3.17.0
 */
public final class NotebookCellKind {
    /**
     * A markup-cell is formatted source that is used for display.
     */
    public static final NotebookCellKind Markup = new NotebookCellKind(1);

    /**
     * A code-cell is source code.
     */
    public static final NotebookCellKind Code = new NotebookCellKind(2);

    private final int value;

    public static NotebookCellKind forValue(int value) {
        switch (value) {
        case 1:
            return Markup;
        case 2:
            return Code;
        default:
            return new NotebookCellKind(value);
        }
    }

    private NotebookCellKind(int value) {
        this.value = value;
    }

    public int getValue() {
        return value;
    }

    @Override
    public boolean equals(Object obj) {
        if (this == obj)
            return true;
        if (obj == null)
            return false;
        if (getClass() != obj.getClass())
            return false;
        NotebookCellKind other = (NotebookCellKind)obj;
        return value == other.value;
    }

    @Override
    public int hashCode() {
        return value;
    }

    @Override
    public String toString() {
        switch (value) {
        case 1:
            return "Markup";
        case 2:
            return "Code";
        default:
            return "Unknown value: " + value;
        }
    }
}

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

No branches or pull requests

3 participants