weavejester
commented
9 years ago Thanks for the patch. I have a few suggestions.
First, if we're adding new middleware, we should ensure it's consistent in terms of naming and argument order. So perhaps instead of:
(with-route-info [(or method :any) (str path)]
(wrap-route-middleware
(fn [request]
(response/render (handler request) request))))We instead have:
(-> (fn [request] (response/render (handler request) request))
(wrap-route-middleware)
(wrap-route-info [(or method :any) (str path)]))Another problem is that your code gets rid of the outer let, so it recreates the route vector each time. I'd prefer if we keep the outer let to make things a little more efficient:
(let [route-info [(or method :any) (str path)]]
(if-method method
(if-route path
(-> (fn [request] (response/render (handler request) request))
(wrap-route-middleware)
(wrap-route-info route-info)))))You could also write wrap-route-info a shade more concisely:
(defn wrap-route-info [handler route-info]
(fn [request]
(handler (assoc request :compojure/route route-info))))There are also a couple of typos in your commit message. The summary line shouldn't end in a period, and you've misspelt "usable". So something more like:
Make matched route info available to middleware
Assoc the :compojure/route data into the request outside the
wrap-route-middleware handler to make it usable by middleware.
mjroghelia
sluukkonen
dpetrovics
The utility of the
:compojure/routekey recently introduced into the request map was limited by the fact that it was unavailable to middleware functions due that functionality being nested inside the wrap-route-middleware handler. This adds an additional wrapper to assoc the matched route into the request before the route middleware is invoked. This makes it possible to implement generic route-based logging centrally in middleware, for example.