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

Generate visitor interfaces for conjure enums #142

Merged
merged 5 commits into from
Jan 9, 2019

Conversation

carterkozak
Copy link
Contributor

Most uses of enums involve a switch or if-else block across values
along the lines of "boolean isTerminalState(enum)" or
"String getDescription(enum)". It's easy to add new enum values
without updating all usages, by using a visitor we catch these
bugs at compile time rather than runtime.

Most uses of enums involve a switch or if-else block across values
along the lines of "boolean isTerminalState(enum)" or
"String getDescription(enum)". It's easy to add new enum values
without updating all usages, by using a visitor we catch these
bugs at compile time rather than runtime.
@carterkozak carterkozak requested a review from a team as a code owner December 25, 2018 12:40
Tests for generated enums
@markelliot
Copy link
Contributor

Mentioned on #134, but I think we should strongly consider dropping enums in favor of unions and/or switching enum code-gen to use the union code-gen, as enums are really a subset of the union functionality.

@rubenfiszel
Copy link
Contributor

rubenfiszel commented Jan 7, 2019

@markelliot One possible migration path is to keep the conjure frontend the same but compile both enum and union to the same internal representation by adding a literal type.

EnumFoo:
  values:
    - a 
    - b
    - c

would be equivalent to:

UnionFoo:
  union:
    - a: literal
    - b: literal
    - c: literal

(can shorten to lit)

and an instance of UnionFoo.a would generate the actual type in the ir: literal<pkg.UnionFoo.a>

Examples of literal types in other languages:

@markelliot
Copy link
Contributor

I think a special “none” type would be cleaner and avoid opening up enums to also be used as constants. It’d also satisfy a common union pain point.

After collapsing the codegen, the union codegen probably needs some much closer inspection for clarity and perf.

@carterkozak
Copy link
Contributor Author

This proposal will cause issues with header and query parameter enum values, we would have to validate that all enum types are lit, and cannot ever receive non-lit values. fwiw I think the use cases for union and enums are fairly distinct, if anything I'd prefer to remove the specifier key from the union type which encroaches on the enums territory.

Copy link
Contributor

@iamdanfox iamdanfox left a comment

Choose a reason for hiding this comment

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

👍 Lots of the arguments for why we recommend visitors for unions also apply to enums, so I think adding visitor codegen here makes a bunch of sense. Whether we want to eventually unify them or not, I think this is a step in the right direction.

I think discussions about whether we should drop enums completely warrants a dedicated ticket on palantir/conjure as this would affect all languages.

@carterkozak
Copy link
Contributor Author

Planning to merge tomorrow unless there are strong reservations.

@bulldozer-bot bulldozer-bot bot merged commit 5994e94 into palantir:develop Jan 9, 2019
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants