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: path parsing issue 2 #379

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

Conversation

martinvibes
Copy link
Contributor

@martinvibes martinvibes commented Jan 31, 2025

closes #366

@martinvibes
Copy link
Contributor Author

gm @ccoVeille kindly review :)

@EwenQuim
Copy link
Member

It seems that is solves #366, not #322. So it's a duplicate of #371.

Honestly, as @NueloSE wasn't answering, it does not bother me.

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.

Good PR, well structured!

Please add tests to show that it works for several patterns, and inform our users of this behaviour in the docs :)

Comment on lines +453 to +461
// Add middleware to strip trailing slash from requests
stripSlashMiddleware := func(next http.Handler) http.Handler {
return http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) {
if len(r.URL.Path) > 1 {
r.URL.Path = strings.TrimRight(r.URL.Path, "/")
}
next.ServeHTTP(w, r)
})
}
Copy link
Member

Choose a reason for hiding this comment

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

Please do not re-define this middleware each time the option is called. Place the middleware elsewhere :) Also, I think the middleware should be placed in Server.globalMiddlewares, see the difference in the docs.

server.go Outdated
Comment on lines 453 to 466
// WithStripTrailingSlash enables stripping of trailing slashes from routes and requests
// Default is false.
//
// Example:
//
// app := fuego.NewServer(
// fuego.WithStripTrailingSlash(),
// )
func WithStripTrailingSlash() func(*Server) {
return func(s *Server) {
// Add OptionStripTrailingSlash to the default route options
s.routeOptions = append(s.routeOptions, OptionStripTrailingSlash())
}
}
Copy link
Member

Choose a reason for hiding this comment

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

Very good! With one Server Option, they will be able to have this feature :)

I think it should look like this:

s.routeOptions = append(s.routeOptions, OptionStripTrailingSlash())
s.globalMiddlewares = append(s.globalMiddlewares, stripTrailingSlashesMiddleware)

@martinvibes
Copy link
Contributor Author

kindly review @EwenQuim

@dylanhitt
Copy link
Collaborator

Edited title so I don't keep getting confused 😄

@dylanhitt dylanhitt changed the title fix: error custom edge case fix: path parsing issue 2 Feb 2, 2025
@martinvibes
Copy link
Contributor Author

😅

@EwenQuim
Copy link
Member

EwenQuim commented Feb 2, 2025

Please add tests to show that it works for several patterns, and inform our users of this behaviour in the docs :)
@EwenQuim

kindly review
@martinvibes after adding absolutely 0 tests

mmmh 🤔

Also, what about this comment and this one ?

Seems that it wasn't really ready lol ;)

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.

Path parsing issue
3 participants