MatchErr is set to ErrNotFound if NotFoundHandler is used #311
Conversation
mux_test.go
Outdated
| matched := r.Match(req, match) | ||
|
|
||
| if matched { | ||
| t.Error("Subrouter should not have matched that") |
There was a problem hiding this comment.
I would add some context - ”subrouter should not have matched: got %v”, route” to aid debugging.
mux_test.go
Outdated
| matched = s.Match(req, match) | ||
| // Now we should get a match | ||
| if !matched { | ||
| t.Error("Subrouter should have matched that") |
mux_test.go
Outdated
|
|
||
| // Now we should get a match | ||
| if !matched { | ||
| t.Error("Subrouter should have matched that") |
|
I've added the matching route where none is expected but one is got. I've also moved the last two test cases above the private functions, which I think is more appropriate. The go fmt issue with travis is bugging me a bit, but afaik it's not my fault. It appeared when I merged upstream :/ |
|
Thanks for this. Let me see why gofmt is failing [only] on tip. |
|
Rebase against master to pull in #312
…On Fri, Nov 3, 2017 at 12:55 PM Roberto Santalla ***@***.***> wrote:
I've added the matching route where none is expected but one is got. I've
also moved the last two test cases above the private functions, which I
think is more appropriate.
The go fmt issue with travis is bugging me a bit, but afaik it's not my
fault. It appeared when I merged upstream :/
—
You are receiving this because you commented.
Reply to this email directly, view it on GitHub
<#311 (comment)>, or mute
the thread
<https://github.com/notifications/unsubscribe-auth/AABIcFRthFq0H5NDllXQu0ATJaDGYz6Sks5sy2-ggaJpZM4QRfiH>
.
|
route.go
Outdated
| @@ -72,7 +72,11 @@ func (r *Route) Match(req *http.Request, match *RouteMatch) bool { | |||
| return false | |||
| } | |||
|
|
|||
| match.MatchErr = nil | |||
| if match.MatchErr == ErrMethodMismatch { | |||
| // We found a non-mismatching route, clear MatchErr | |||
There was a problem hiding this comment.
non-mismatching is a bit of a double negative :) how about just "matching" so it's less confusing?
There was a problem hiding this comment.
Hm, I used that wording to make clear that the ErrMethodMismatch found on the previous iteration no longer applies. But I guess I can reword it a bit.
I'll do it tomorrow tho, its 2AM here and I'm quite asleep.
|
LGTM other than comment |
|
Thanks! I don't mean to be too nitpicky, but the code base is already pretty hard to follow so any little bit helps. |
|
@kisielk No prob! I'm not a native speaker, so I can make some grammar mistakes here and there. Also, I'd do the same on projects I mantain :P |
As discussed in #293, it should be useful if
RouteMatch.MatchErrwere set to an error if no match is found, even if some subrouters have a customNotFoundHandler.