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

Gin compat #260

Draft
wants to merge 7 commits into
base: main
Choose a base branch
from
Draft

Gin compat #260

wants to merge 7 commits into from

Conversation

EwenQuim
Copy link
Member

@EwenQuim EwenQuim commented Dec 9, 2024

Fixes #241 : it works!!! 🎉

🚧 Ultra-WIP as it is not ready for production.

Enables the way for Echo & other frameworks.

The (challengeable) goal here is not to support both frameworks but to allow smooth transition towards stdlib.

Copy link
Contributor

Choose a reason for hiding this comment

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

fuegogin is a strange name either for folder name and package

What about using one of these?

extra/adapter/gin.go
extra/gin/adapter.go

Copy link
Member Author

Choose a reason for hiding this comment

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

gin as a Fuego Extra package name will obviously collide with gin the router.

It still needs to reference gin. I don't have good ideas. I'll ask ChatGPT 😆

extra/fuegogin/adaptor_test.go Outdated Show resolved Hide resolved

type ContextNoBody = ContextWithBody[any]

// Body implements fuego.Ctx.
Copy link
Contributor

Choose a reason for hiding this comment

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

Use godoc

Suggested change
// Body implements fuego.Ctx.
// Body implements [fuego.Ctx].

But also, this could be better

Suggested change
// Body implements fuego.Ctx.
// Body satisfies [fuego.Ctx].

Body itself doesn't implements the interface, it helps to comply and satisfy it

Comment on lines +30 to +34

// Cookie implements fuego.Ctx.
func (c *ContextWithBody[B]) Cookie(name string) (*http.Cookie, error) {
panic("unimplemented")
}
Copy link
Contributor

Choose a reason for hiding this comment

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

Define a sentinel error, you will be able to catch in tests with a type assertion, then check its value with an errors.Is

Suggested change
// Cookie implements fuego.Ctx.
func (c *ContextWithBody[B]) Cookie(name string) (*http.Cookie, error) {
panic("unimplemented")
}
var ErrUnimplemented = errors.New("unimplemented")
// Cookie implements fuego.Ctx.
func (c *ContextWithBody[B]) Cookie(name string) (*http.Cookie, error) {
panic(ErrUnimplemented)
}

extra/fuegogin/lib/lib.go Show resolved Hide resolved
mux.go Outdated
Comment on lines 258 to 262
description := "#### Controller: \n\n`" +
handler + "`"

Copy link
Contributor

Choose a reason for hiding this comment

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

This would be more readable I think

Suggested change
description := "#### Controller: \n\n`" +
handler + "`"
description := fmt.Sprintf ("#### Controller: \n\n`%s`", handler)

description += "\n\n#### Middlewares:\n"

for i, fn := range middlewares {
description += "\n- `" + FuncName(fn) + "`"
Copy link
Contributor

Choose a reason for hiding this comment

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

Same logic

mux.go Outdated Show resolved Hide resolved
openapi.go Outdated
}

if route.Operation.OperationID == "" {
route.Operation.OperationID = route.Method + "_" + strings.ReplaceAll(strings.ReplaceAll(route.Path, "{", ":"), "}", "")
Copy link
Contributor

Choose a reason for hiding this comment

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

Use a func to clean operation ID, this code is duplicated

option.go Outdated
Comment on lines 280 to 288
// Description appends a description to the route.
// By default, the description is set by Fuego with some info,
// like the controller function name and the package name.
func OptionOverrideDescription(description string) func(*BaseRoute) {
return func(r *BaseRoute) {
r.OverrideDescription = true
r.Operation.Description = description
}
}
Copy link
Contributor

Choose a reason for hiding this comment

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

Didn't I provide a feedback for this?

About the fact the code is a copy pasta, and should be

// OptionOverrideDescription overrides the default description set by Fuego.
// By default, the description is set by Fuego with some info,
// like the controller function name and the package name.

@ccoVeille
Copy link
Contributor

ccoVeille commented Dec 14, 2024

It's a draft, but I have feedbacks

Screenshot_20241214_105816_Firefox.jpg

@EwenQuim
Copy link
Member Author

Thanks for the memes @ccoVeille 😆

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.

Gin compatibility
2 participants