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

Case key path support #323

Open
wants to merge 2 commits into
base: main
Choose a base branch
from
Open

Case key path support #323

wants to merge 2 commits into from

Conversation

stephencelis
Copy link
Member

Adds support for case key paths. While parsers do not seem benefit from some features of case key paths (they can't be abbreviated), they can at least be used more performantly without the reflection associated with the old style of case paths.

@ShivaHuang
Copy link

Hi @stephencelis,

Would you complete this PR?
Recently I wrote some code exactly the same with this one.

I found it helps to write more clear code.

Lets say we have the following route enum:

@CasePathable
enum Route: Equatable {
    case home
    case user(User.ID)
}

Before, we need to write this way:

OneOf {
    Route(.case(Route.home)) {
        Path { "home" }
    }
    Route(.case(Route.user)) {
        Path { "user"; UUID.parser() }
    }
}

and with the change in this PR, we can write:

OneOf(output: Route.self) {
    Route(.case(\.home)) {
        Path { "home" }
    }
    Route(.case(\.user)) {
        Path { "user"; UUID.parser() }
    }
}

and the auto-completion is working great with case key path, so I think it's worth to do the improvements.

@stephencelis
Copy link
Member Author

stephencelis commented Jun 12, 2024

@ShivaHuang Part of the reason we haven't pushed through this is that case key paths can't be inferred the way you want. The root Enum type is not known in the builder, and so you'd have to do this:

 OneOf(output: MyRoute.self) {
-    Route(.case(\.home)) {
+    Route(.case(\MyRoute.Cases.home)) {
         Path { "home" }
     }
-    Route(.case(\.user)) {
+    Route(.case(\MyRoute.Cases.user)) {
         Path { "user"; UUID.parser() }
     }
 }

This isn't to say we shouldn't eventually land this PR, but we haven't prioritized it since it isn't an ergonomic improvement, and in fact is a bit more verbose than it was previously due to the need of .Cases.

Edit: Ah, catching up you're relying on the Output change for OneOf. That works, but it's a bummer that it would be limited to OneOf builders, and may confuse folks. We'll keep thinking about it, though! Thanks for the reminder!

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