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

Add full enum interoperability #10

Merged
merged 4 commits into from
Aug 28, 2020
Merged

Add full enum interoperability #10

merged 4 commits into from
Aug 28, 2020

Conversation

alexvanyo
Copy link
Collaborator

@alexvanyo alexvanyo commented Aug 22, 2020

This PR completes the goal of having sealed enums be "strictly more powerful" than enums.

It accomplishes this in two ways:

  • SealedEnum now specifies nameOf and valueOf functions, which use the simple names of the objects joined by _.

  • Previously, there was an interoperability layer to create SealedEnums from normal enums.

    Now, there is now the reverse layer: generating enum classes in addition to generating SealedEnum classes. If the new optional generateEnum parameter of @GenSealedEnum is true, then a regular enum class will be generated that is isomorphic to the sealed enum. This enum class can be specified in annotations, or passed to any interface built around normal enum classes. The bijection is implemented by an EnumForSealedEnumProvider interface, which also contains the Class of the enum.

    Normal enum classes are more restrictive than sealed classes, but they can implement interfaces. Thus, the generated enum class implements all of the interfaces that the sealed class does via delegation to the sealed objects.

To make the above possible, generics are also now handled with more nuance. In short, whenever possible invariant types are used. If there are covariant or contra-variant types, they will be star-projected when possible, and when that isn't possible (such as the direct arguments for an interface that the enum class should implement), that interface is left out.

Edit:

Through the review, this PR evolved to also include the following changes:

  • @GenSealedEnum must now be applied to the companion object of the sealed class.
  • The above allows generation of extension properties and methods on the sealed class and its companion object to allow for nicer syntax for accessing the properties in a more enum-like way
  • The autogeneration feature has been removed.

The review and working on this PR also surfaced 5 issues: #11 , #12 , #13 , #14 and #15 .

@livefront-ci
Copy link

livefront-ci commented Aug 22, 2020

🧛 Project Code Coverage: 100.00%

Coverage of Modified Files:

File Coverage
SealedEnum.kt 100.00%

Modified Files Not Found In Coverage Report:

CamelCase.kt
CreateSealedEnumFromEnumTests.kt
EmptySealedClassTests.kt
EnumForSealedEnumProvider.kt
EnumForSealedEnumTypeSpec.kt
EnumSealedObjectPropertySpec.kt
EnvironmentsSealedEnumTests.kt
GenSealedEnum.kt
GenericSealedClassTests.kt
NestedClassTests.kt
OneObjectSealedClassTests.kt
OutsideSealedClassTests.kt
README.md
SealedClassExtensions.kt
SealedClassHierarchyTests.kt
SealedClassNode.kt
SealedEnumEnumPropertySpec.kt
SealedEnumFileSpec.kt
SealedEnumFileSpecTests.kt
SealedEnumNamePropertySpec.kt
SealedEnumOrdinalPropertySpec.kt
SealedEnumProcessor.kt
SealedEnumProcessorTests.kt
SealedEnumSealedEnumPropertySpec.kt
SealedEnumTypeSpec.kt
SealedEnumValueOfFunSpec.kt
SealedEnumValuesPropertySpec.kt
SealedEnumWithAbstractBaseClassesTests.kt
SealedEnumWithEnumProvider.kt
SealedEnumWithInterfacesTests.kt
SuperInterfaces.kt
SuperInterfacesTests.kt
TraversalOrderTests.kt
TwoObjectSealedClassTests.kt
Unique.kt
WildcardedTypeParameters.kt
build.gradle.kts
gradle-wrapper.properties

Codebase cunningly covered by count Shroud 🧛

🧛 Project Code Coverage: 98.21%

Coverage of Modified Files:

File Coverage
CamelCase.kt 100.00%
EnumForSealedEnumTypeSpec.kt 100.00%
EnumSealedObjectPropertySpec.kt 100.00%
SealedClassExtensions.kt 100.00%
SealedClassNode.kt 100.00%
SealedEnumEnumPropertySpec.kt 100.00%
SealedEnumFileSpec.kt 100.00%
SealedEnumNamePropertySpec.kt 100.00%
SealedEnumOrdinalPropertySpec.kt 100.00%
SealedEnumProcessor.kt 96.24%
SealedEnumSealedEnumPropertySpec.kt 100.00%
SealedEnumTypeSpec.kt 100.00%
SealedEnumValueOfFunSpec.kt 100.00%
SealedEnumValuesPropertySpec.kt 100.00%
SuperInterfaces.kt 88.41%
Unique.kt 100.00%
WildcardedTypeParameters.kt 88.24%

Modified Files Not Found In Coverage Report:

CreateSealedEnumFromEnumTests.kt
EmptySealedClassTests.kt
EnumForSealedEnumProvider.kt
EnvironmentsSealedEnumTests.kt
GenSealedEnum.kt
GenericSealedClassTests.kt
NestedClassTests.kt
OneObjectSealedClassTests.kt
OutsideSealedClassTests.kt
README.md
SealedClassHierarchyTests.kt
SealedEnum.kt
SealedEnumFileSpecTests.kt
SealedEnumProcessorTests.kt
SealedEnumWithAbstractBaseClassesTests.kt
SealedEnumWithEnumProvider.kt
SealedEnumWithInterfacesTests.kt
SuperInterfacesTests.kt
TraversalOrderTests.kt
TwoObjectSealedClassTests.kt
build.gradle.kts
gradle-wrapper.properties

Codebase cunningly covered by count Shroud 🧛

Generated by 🚫 Danger

@alexvanyo alexvanyo force-pushed the av/full-enum-interop branch 2 times, most recently from 5833be0 to 90910a7 Compare August 24, 2020 14:20
Copy link

@cdflynn cdflynn left a comment

Choose a reason for hiding this comment

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

Before you read through my comments:
Let me say that this is, and I can't stress this enough, awesome.

There's some open questions in my mind that I didn't write below, having entirely to do with how the user experience plays out. I started to jot down some suggestions for the README, but I might follow up with a PR of my own for that after I tinker some more.

@alexvanyo alexvanyo requested a review from cdflynn August 24, 2020 20:24
@alexvanyo
Copy link
Collaborator Author

alexvanyo commented Aug 25, 2020

@cdflynn The latest commit of 7dda726 adds all of the extensions properties and methods discussed above.

I ended up making the companion object required, by enforcing that the companion object of the sealed class is annotated with @GenSealedEnum instead of the sealed class itself. (Tangent, this is actually a funny return to how it originally was in the first incarnation of sealed enum).

On warning, the code and tests are almost all up-to-date, but I haven't reworked the README or the larger KDocs yet. Let me know what you think!

Copy link

@cdflynn cdflynn left a comment

Choose a reason for hiding this comment

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

Love it. The properties existing on the user-defined class (by extensions) feels really good to use.

I didn't say this yesterday but I also like the fact that the library can jump through extra hoops to work with generics and parameterized types, and the typical call site can still reclaim any type information declared on the constituent objects.

for (value in MySealedClass.values /* List<MySealedClass<*>> */) {
  when (value) {
    AnObject -> // All parameterized types are explicit for this object
    //...
  }
}

Just a general FYI, I get one new warning when doing a clean build:

$ ./gradlew clean
$ ./gradlew assemble

> Task :sealedenumprocessor:kaptKotlin
warning: The following options were not recognized by any processor: '[sealedenum.autoGenerateSealedEnums, kapt.kotlin.generated]'
BUILD SUCCESSFUL in 11s

Thanks @alexvanyo, this is really cool to see in action. You're crushing this.

@alexvanyo
Copy link
Collaborator Author

c391838 removes the auto-generation option, which allows a bit of logic to be simplified. (No more need for SealedEnumSeed)

This also fixes the compilation warnings due to unrecognized options.

The final step for this PR will be to update all of the KDocs and README to reflect the new methods and behavior, and I will add issues for the additional warnings that would be nice to have to tighten the feedback loop.

@alexvanyo
Copy link
Collaborator Author

@byencho @cdflynn This should be ready to go for another review! I responded to all feedback, and all KDocs and documentation should be up-to-date with the latest changes.

I opened up 5 issues that this PR didn't get to: #11 , #12 , #13 , #14 and #15 . Two of these deal with adding more warnings, and the other 3 are edge cases that I discovered while testing.

Copy link

@cdflynn cdflynn left a comment

Choose a reason for hiding this comment

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

I think we're at a good stopping point here, and the issues capture the follow-ups from our discussion.

@alexvanyo alexvanyo merged commit c319a37 into master Aug 28, 2020
@alexvanyo
Copy link
Collaborator Author

Thanks @cdflynn and @byencho for the reviews and feedback! Really proud of how far this has come.

@alexvanyo alexvanyo deleted the av/full-enum-interop branch August 28, 2020 16:36
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