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

fix: NewRoute/NewBaseRoute to take engine not just OpeAPI for accepted content types #334

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

Conversation

dylanhitt
Copy link
Collaborator

@dylanhitt dylanhitt commented Jan 7, 2025

Relates to #333

Somewhere along implementing support for different engines this was lost. This is a bit disruptive as it's changing the NewRoute/NewBaseRoute. The other option would be to place acceptedContentTypes on our OpenAPI struct I could go either way. We wouldn't be changing the NewRoute and NewBaseRoute API in the case of the later, but again we'd need to support yet another nested structure with it's own options. I think flattening in the Engine is OK, I guess 🤷.

So the options are

  1. This PR. Just be fine with configuration occurring on the engine struct
  2. Place acceptedContentTypes in the OpenAPI struct. We can could then do something like
func WithRequestContentType(consumes ...string) func(*Engine) {
	return func(e *Engine) { e.OpenAPI.acceptedContentTypes = consumes }
}
  1. Place acceptedContentTypes in the OpenAPI struct. And expose a WithOpenAPIOptions. At this point we're nesting pretty deep...
  2. This could exist on the OpenAPIConfig struct as well. Then engine would still need to be passed to the New*Route funcs.

Cheers

server.go Show resolved Hide resolved
server_test.go Show resolved Hide resolved
func NewBaseRoute(method, path string, handler any, openapi *OpenAPI, options ...func(*BaseRoute)) BaseRoute {
func NewBaseRoute(method, path string, handler any, e *Engine, options ...func(*BaseRoute)) BaseRoute {
Copy link
Member

Choose a reason for hiding this comment

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

But do we really need this? For future compatibility maybe?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

I figure we can just deprecate this function. You are just talking about the function right? Deprecating the Struct of BaseRoute would be a fairly large change which we'd probably want a separate PR for.

Copy link
Member

Choose a reason for hiding this comment

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

No, I was talking about changing the *OpenAPI param to *engine

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Oh. It fixed the WithContentTypes option. I think the line you're missing is:

https://github.com/go-fuego/fuego/pull/334/files#diff-6b1e829598248403d0c671996c16c10f8a2d8db33f96b6db158654b78103dec4R31

see the AcceptedContentTypes: e.acceptedContentTypes,

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Hence my 4 questions where do we want to put acceptedContentTypes and how do we want to provide an option to configure it 😄

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Let me know if that clarifies things. I personally don't mind acceptedContentTypes existing on the Engine. At the very least I want avoid having to support a set of options for NewOpenAPI as we're getting to be pretty nested at that point.

So my preference is: 1 or 2

@dylanhitt dylanhitt force-pushed the BREAKING/engine.WithRequestContentType.fix branch from 8ed6f35 to 1529578 Compare January 8, 2025 17:47
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