fix for issue #429 #431
fix for issue #429 #431
Conversation
|
Can you please provide:
- A test
- A clearer issue description / comment around the fix.
…On Thu, Dec 20, 2018 at 12:17 PM tomare ***@***.***> wrote:
fix for issue #429 <#429>
------------------------------
You can view, comment on, or merge this pull request online at:
#431
Commit Summary
- Update route.go
File Changes
- *M* route.go <https://github.com/gorilla/mux/pull/431/files#diff-0>
(2)
Patch Links:
- https://github.com/gorilla/mux/pull/431.patch
- https://github.com/gorilla/mux/pull/431.diff
—
You are receiving this because you are subscribed to this thread.
Reply to this email directly, view it on GitHub
<#431>, or mute the thread
<https://github.com/notifications/unsubscribe-auth/AABIcDSOY12WWgTQvkdWYksxMcRQmtZYks5u6_BWgaJpZM4Zc2NW>
.
|
|
Condition:
Issue and reason: Fix: |
|
💚 thank you for this patch. |
middleware_test.go
Outdated
|
|
||
| router.ServeHTTP(rw, req) | ||
| if rw.Body.String() != first { | ||
| t.Fatal("First Middleware doesn't work") |
There was a problem hiding this comment.
t.Failf("Middleware did not run: expected %s middleware to write a response (got %s)", first, body)
middleware_test.go
Outdated
|
|
||
| router.ServeHTTP(rw, req) | ||
| if rw.Body.String() != second { | ||
| t.Fatal("Second Middleware doesn't work") |
There was a problem hiding this comment.
t.Failf("Middleware did not run: expected %s middleware to write a response (got %s)", second, body)
route.go
Outdated
| @@ -43,6 +43,8 @@ func (r *Route) Match(req *http.Request, match *RouteMatch) bool { | |||
| return false | |||
| } | |||
|
|
|||
| //reset MatchErr | |||
There was a problem hiding this comment.
Tell a story / why - it's obvious that we reset it :)
Instead, say: "Set MatchErr to nil to prevent subsequent matching subrouters from failing to run middleware. If not reset, the middleware would see a non-nil MatchErr and be skipped, even when there was a matching route"
tomare
force-pushed
the
master
branch
2 times, most recently
from
446e65c to
e0ab5d0
Compare
add middlewareOnMultiSubrouter test remove blank line
|
::bump:: I'd really love to see this merged. Is the only thing we're waiting on a small addition to the test (to add a third non-matching route)? |
|
Yep - the tests need to be comprehensive. It’s clear that our already large test suite doesn’t cover all behaviours, especially with complex subrouter + middleware interactions. The burden on PRs to provide more complete test cases will only increase 😉 |
|
Looks like the comment was addressed, just didn’t see a reply comment. Let me review in full at a computer later today before merging. |
|
Thanks @tomare! |
|
I don't see this in master branch. When will this be released? How do I go get it? |
It was released and changed on later commit. |
|
@tomare - Sorry, can you provide more info? Why was it changed? Is there an associated issue, so I can read up on it? |
fix for issue #429