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

Open enumerations #1137

Merged
merged 16 commits into from
Aug 10, 2023
Merged

Open enumerations #1137

merged 16 commits into from
Aug 10, 2023

Conversation

lewisjkl
Copy link
Contributor

@lewisjkl lewisjkl commented Aug 7, 2023

This PR introduces open enumerations that are codegenerated with an extra Unknown case for handling values that are not defined in the specification. The presence of the alloy#openEnum trait on an enum or intEnum shape will trigger this to take place in the codegen.

The main need for this is that AWS treats enums as open in several places in their SDKs/APIs where as we treat them as closed always (in the past). This PR will allow for maintaining the default of closed enumerations, while also enabling interop with AWS and the like.

@lewisjkl
Copy link
Contributor Author

lewisjkl commented Aug 7, 2023

Still going to add docs and changelog and such, just working on getting CI happy in the meantime

@lewisjkl
Copy link
Contributor Author

lewisjkl commented Aug 7, 2023

One thing I still need to handle are collisions on the name Unknown since that could be a name that someone uses for an enum. Not sure the best thing to do in that case, but leaning toward making our Unknown into $Unknown or something like that. Will create an example and solve it tomorrow.

@kubukoz
Copy link
Member

kubukoz commented Aug 7, 2023

That's interesting and well-timed as well, I was just thinking of how I'd support open enums in smithy-playground :)

will try to review after the weekend as I'm OOO

@Baccata
Copy link
Contributor

Baccata commented Aug 8, 2023

but leaning toward making our Unknown into $Unknown or something like that. Will create an example and solve it tomorrow.

I'm happy for you to roll with this 👍

@lewisjkl
Copy link
Contributor Author

lewisjkl commented Aug 8, 2023

Ultimately decided to render all unknown enum members with a leading $. This simplifies things quite a bit and will make it very clear that the member is coming from an open enum and not from the spec. However, I'm happy to change it to only use a $ when there is a collision if others think that is better.

@lewisjkl
Copy link
Contributor Author

lewisjkl commented Aug 8, 2023

So apparently using the -Xlint:missing-interpolator option with scala 2.12 means that it will warn on the following:

final case class $Unknown()

So I changed it to be:

final case class $$Unknown()

which works fine, but it is obviously kind of ugly.. so open to other suggestions.

@lewisjkl lewisjkl marked this pull request as ready for review August 8, 2023 23:04
@kubukoz
Copy link
Member

kubukoz commented Aug 8, 2023

Frankly if it's just 2.12 I would keep the single $ and tell users who are stuck on 2.12 to ignore the warning in the build.

@Baccata
Copy link
Contributor

Baccata commented Aug 9, 2023

I agree with Jakub

@Baccata
Copy link
Contributor

Baccata commented Aug 9, 2023

Great work on this ! The only thing that bothers me is the $$ to make 2.12 happy. We can remove this linter from 2.12 and roll with a single $

@lewisjkl lewisjkl merged commit 1954ba9 into series/0.18 Aug 10, 2023
@lewisjkl lewisjkl deleted the open-enums branch August 10, 2023 17:15
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.

3 participants