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

add Lookup and AllowedMethods methods, change custom NotFound semantics #47

Open
wants to merge 5 commits into
base: master
Choose a base branch
from

Conversation

mna
Copy link

@mna mna commented Jul 21, 2016

Hello,

I've used something like this fork of pat for a while, it adds two methods that can be very useful when building middleware chains:

  • AllowedMethods : returns the allowed methods for a given path, useful esp. for a CORS middleware.
  • Lookup : returns the matching registered handler for a given method and path, useful if in a middleware you want to check if the request actually has a handler before starting setting up the usual chain (e.g. logging, panic recover, cors, etc.)

I also made another change that may be a bit trickier to merge, I found it quite weird that setting a custom NotFound handler on the mux changed the way the 404 was returned. If there's no NotFound and a path has other methods registered, it returns a 405, and the 404 is only returned if there's no match and no other methods for the path. Once you set a custom NotFound handler, the behaviour changes and the 405 is not returned anymore (presumably it's because the custom not found should handle it? Still seems weird/unexpected).

So this PR changes the behaviour so that setting a custom NotFound still behaves the same way as without it - it's just called if no 405 was returned. A case could probably be made to add support for a custom MethodNotAllowed handler, but I didn't add this.

I'm fine with keeping this stuff in my fork if you're not interested in adding more API surface to the package, no worries.

Thanks,
Martin

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.

1 participant