[Strongly Typed Params] Typed params in context #406
Conversation
EwenQuim
force-pushed
the
typed-params-in-context
branch
from
f347536 to
b227d91
Compare
EwenQuim
force-pushed
the
typed-params-in-context
branch
from
b5eb80c to
ef177ea
Compare
|
Is this ready for review? |
|
Yes, it is only a first step as it does not implement the solution but it paves the way to support typed params in a backward-compatible way (except for testing with |
|
Got it. Will review soon. |
EwenQuim
changed the title
There was a problem hiding this comment.
I believe go work and all the go mods should probably be bumped on the commit titled Update go version to 1.24
ctx.go
Outdated
| _ Context[any, any] = &netHttpContext[any, any]{} // Check that ContextWithBody implements Ctx. | ||
| _ ContextWithBody[any] = &netHttpContext[any, any]{} // Check that ContextWithBody implements Ctx. | ||
| _ ContextWithBody[string] = &netHttpContext[string, any]{} // Check that ContextWithBody implements Ctx. | ||
| _ ValidableCtx = &netHttpContext[any, any]{} // Check that ContextWithBody implements ValidableCtx. |
There was a problem hiding this comment.
I think there is a lot of copy pasta right here maybe with the comments? Probably should go above as well.
|
Hey, just discovered this project yesterday and I'm really impressed :). Forgive me if any of my comments/questions are uninformed. Will Param type support be an effective replacement for the route options All-in-all, I'm excited for this update; I think Param types fit the fuego philosophy much better than declarations via route options - leave it to the function signature to define params instead of the route registering logic (again, forgive me if I'm speaking out of turn). Also, I'm happy to help once I get more familiar with the codebase - if you'll have me. |
|
Hi @dylanhitt and @crunk1 ! Typed params will not replace the Options for a long time, you can safely use them. I know it creates a "two ways of doing the same thing" but one is very strict and the other is (too much?) loosely coupled, and users may want to choose between the 2. |
|
@EwenQuim I'm currently building (in my own project) a wrapper around route registration that translates Param struct types into calls to Thanks for the warm welcome from you and @dylanhitt. |
EwenQuim
force-pushed
the
typed-params-in-context
branch
3 times, most recently
from
0afc291 to
69459c0
Compare
|
Ready for review @ccoVeille @dylanhitt |
| // TODO: implementation must pass this test | ||
| // require.NotEmpty(t, params) | ||
| // require.Equal(t, 456, params.ID) | ||
| // require.Equal(t, "hello", params.Other) | ||
| // require.Equal(t, "application/json", params.ContentType) |
There was a problem hiding this comment.
Something left behind?
There is gocritic rule about reporting commented out code
There was a problem hiding this comment.
No, something that must be implemented in the future
|
I do plan on reviewing this sometime this week by the way. Day job is getting in the way this week. |
|
I don't suppose this will help with your current direction, but I have taken a crack at writing a wrapper library that supports Param types, piggy-backing on fuego's route options: https://github.com/crunk1/xfuego. It populates routes' params types on incoming requests. example: https://github.com/crunk1/xfuego/blob/main/example/main.go |
|
Omg @crunk1 that's incredible work!!! I'm definitely having a look at your code |
|
@dylanhitt Happy to take your review when you have some time :) |
Co-authored-by: ccoVeille <3875889+ccoVeille@users.noreply.github.com>
EwenQuim
force-pushed
the
typed-params-in-context
branch
from
bf11ff2 to
d993e71
Compare
First step towards #356