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

refactor: move accept header from mux to server options #324

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

Conversation

ekumamatthew
Copy link
Contributor

@ekumamatthew ekumamatthew commented Dec 30, 2024

refactor: move accept header from mux to server options
fixes #266

Copy link
Member

@EwenQuim EwenQuim left a comment

Choose a reason for hiding this comment

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

Thank you for this PR! 🙏🏼

server.go Outdated Show resolved Hide resolved
@EwenQuim
Copy link
Member

EwenQuim commented Jan 9, 2025

Fixes #266

@@ -402,10 +402,10 @@ func TestGroupParams(t *testing.T) {
t.Log(document.Paths.Find("/").Get.Parameters[0].Value.Name)
require.Len(t, document.Paths.Find("/").Get.Parameters, 1)
require.Equal(t, document.Paths.Find("/").Get.Parameters[0].Value.Name, "Accept")
Copy link
Collaborator

Choose a reason for hiding this comment

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

Not your change, but we may want to check if require.NotNil(t, document.Paths.Find("/").Get.Parameters[0].Value)

Also could probably change all the require.Equal below to assert.

Copy link
Collaborator

Choose a reason for hiding this comment

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

This is just a nitpick feel free to resolve

@EwenQuim
Copy link
Member

Please rebase your branch on main, it is quite outdated :)

Thanks for the PR!

@EwenQuim
Copy link
Member

Please fix the tests.

I think you can run make golden-update or something like that to update the golden file. But it's weird, it should not change as it is just an internal refactoring

@ccoVeille ccoVeille changed the title refactor: move accept header form mux to server options refactor: move accept header from mux to server options Jan 14, 2025
route := fuego.Get(s, "/test", func(c fuego.ContextNoBody) (string, error) {
name := c.QueryParam("name")
age := c.QueryParamInt("age")
isok := c.QueryParamBool("is_ok")
Copy link
Collaborator

Choose a reason for hiding this comment

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

nitpicking: I would like to suggest

Suggested change
isok := c.QueryParamBool("is_ok")
isOK := c.QueryParamBool("is_ok")

Comment on lines +38 to +41
require.Equal(t, "Age", route.Params["age"].Description)
require.True(t, route.Params["age"].Nullable)
require.Equal(t, 18, route.Params["age"].Default)
require.Equal(t, "integer", route.Params["age"].GoType)
Copy link
Collaborator

Choose a reason for hiding this comment

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

Suggested change
require.Equal(t, "Age", route.Params["age"].Description)
require.True(t, route.Params["age"].Nullable)
require.Equal(t, 18, route.Params["age"].Default)
require.Equal(t, "integer", route.Params["age"].GoType)
require.Contains(t, route.Params, "age")
require.Equal(t, "Age", route.Params["age"].Description)
require.True(t, route.Params["age"].Nullable)
require.Equal(t, 18, route.Params["age"].Default)
require.Equal(t, "integer", route.Params["age"].GoType)

Or, maybe you could do this, the readability will be increased

Suggested change
require.Equal(t, "Age", route.Params["age"].Description)
require.True(t, route.Params["age"].Nullable)
require.Equal(t, 18, route.Params["age"].Default)
require.Equal(t, "integer", route.Params["age"].GoType)
require.Contains(t, route.Params, "age")
paramAge := route.Params["age"]
require.Equal(t, "Age", paramAge.Description)
require.True(t, paramAge.Nullable)
require.Equal(t, 18, paramAge.Default)
require.Equal(t, "integer", paramAge.GoType)

Comment on lines +32 to +36
require.Equal(t, "Name", route.Params["name"].Description)
require.True(t, route.Params["name"].Required)
require.Equal(t, "hey", route.Params["name"].Default)
require.Equal(t, "you", route.Params["name"].Examples["example1"])
require.Equal(t, "string", route.Params["name"].GoType)
Copy link
Collaborator

Choose a reason for hiding this comment

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

This would be safer if something is refactored, it won't panic

Suggested change
require.Equal(t, "Name", route.Params["name"].Description)
require.True(t, route.Params["name"].Required)
require.Equal(t, "hey", route.Params["name"].Default)
require.Equal(t, "you", route.Params["name"].Examples["example1"])
require.Equal(t, "string", route.Params["name"].GoType)
require.Contains(t, route.Params, name)
require.Equal(t, "Name", route.Params["name"].Description)
require.True(t, route.Params["name"].Required)
require.Equal(t, "hey", route.Params["name"].Default)
require.Contains(t, route.Params["name"].Examples, "example1")
require.Equal(t, "you", route.Params["name"].Examples["example1"])
require.Equal(t, "string", route.Params["name"].GoType)

Copy link
Collaborator

Choose a reason for hiding this comment

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

Also this would be more readable

Suggested change
require.Equal(t, "Name", route.Params["name"].Description)
require.True(t, route.Params["name"].Required)
require.Equal(t, "hey", route.Params["name"].Default)
require.Equal(t, "you", route.Params["name"].Examples["example1"])
require.Equal(t, "string", route.Params["name"].GoType)
require.Contains(t, route.Params, name)
paramName := route.Params["name"]
require.Equal(t, "Name", paramName.Description)
require.True(t, paramName.Required)
require.Equal(t, "hey", paramName.Default)
require.Contains(t, paramName.Examples, "example1")
require.Equal(t, "you", paramName.Examples["example1"])
require.Equal(t, "string", paramName.GoType)

require.Equal(t, document.Paths.Find("/api/test").Get.Parameters[0].Value.Name, "X-Test-Header")
require.Equal(t, document.Paths.Find("/api/test2").Get.Parameters[1].Value.Name, "Accept")
require.Equal(t, document.Paths.Find("/api/test2").Get.Parameters[0].Value.Name, "X-Test-Header")
require.Equal(t, document.Paths.Find("/api/test").Get.Parameters[0].Value.Name, "Accept")
Copy link
Collaborator

Choose a reason for hiding this comment

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

Not related to this PR, but something like this should be added to avoid panics if there is a refactoring

Suggested change
require.Equal(t, document.Paths.Find("/api/test").Get.Parameters[0].Value.Name, "Accept")
require.Len(t, document.Paths.Find("/api/test").Get.Parameters, 2)
require.Equal(t, document.Paths.Find("/api/test").Get.Parameters[0].Value.Name, "Accept")

@@ -376,8 +376,8 @@ func TestGroupHeaderParams(t *testing.T) {
require.Equal(t, "test-value", route.Operation.Parameters.GetByInAndName("header", "X-Test-Header").Description)

document := s.OutputOpenAPISpec()
require.Equal(t, document.Paths.Find("/api/test").Get.Parameters[1].Value.Name, "Accept")
require.Equal(t, document.Paths.Find("/api/test").Get.Parameters[0].Value.Name, "X-Test-Header")
require.Equal(t, document.Paths.Find("/api/test").Get.Parameters[0].Value.Name, "Accept")
Copy link
Collaborator

Choose a reason for hiding this comment

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

Same about testing Get.Parameters with require.Len

And in the next tests you are updating

@ekumamatthew
Copy link
Contributor Author

ekumamatthew commented Jan 14, 2025

Please fix the tests.

I think you can run make golden-update or something like that to update the golden file. But it's weird, it should not change as it is just an internal refactoring

goldenmake

@dylanhitt
Copy link
Collaborator

#324 (comment)

Is that updating that file for you?

@dylanhitt
Copy link
Collaborator

Damn, it isn't. Hmm something is going on here.

@dylanhitt
Copy link
Collaborator

Oh nevermind the test s are failing cause their are two functions named TestParams

@dylanhitt
Copy link
Collaborator

Also the change to golden is the ordering so that should be fine.

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.

Remove hardcoded Accept header
4 participants