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 route information to request. #140

Merged
merged 1 commit into from Jul 5, 2015

Conversation

Projects
None yet
2 participants
@Christopher-Bui
Contributor

Christopher-Bui commented Jun 2, 2015

Feature for #82 and subsumes #93

@@ -102,6 +102,12 @@
((mw handler) request)
(handler request))))
(defn- add-route-information

This comment has been minimized.

@weavejester

weavejester Jun 2, 2015

Owner

assoc-route-info would be a name more in keeping with the other functions in the namespace.

@@ -102,6 +102,12 @@
((mw handler) request)
(handler request))))
(defn- add-route-information
"Returns the request with the matched route assoc'd onto it as a

This comment has been minimized.

@weavejester

weavejester Jun 2, 2015

Owner

This is a private function, so the docstring can be removed.

"Returns the request with the matched route assoc'd onto it as a
vector of request method and the matched route."
[request path]
(assoc request :route [(:request-method request) (:source path)]))

This comment has been minimized.

@weavejester

weavejester Jun 2, 2015

Owner

Use (str path) rather than (:source path).

This comment has been minimized.

@weavejester

weavejester Jun 2, 2015

Owner

Also, you can't use :request-method, as it doesn't necessarily reflect the route method. Instead, pass method as an argument in.

@@ -110,7 +116,8 @@
(if-route path
(wrap-route-middleware
(fn [request]
(response/render (handler request) request))))))
(response/render (handler (add-route-information request path))

This comment has been minimized.

@weavejester

weavejester Jun 2, 2015

Owner

It would be better to pre-calculate the route info, e.g.

(let [route-info [(or method :any) (str path)]] ...)
@Christopher-Bui

This comment has been minimized.

Contributor

Christopher-Bui commented Jun 3, 2015

Couple of questions:

Is the reason you say to precompute the route info to decouple the add-route-information function from needing to know about the stuff? Secondly, after doing that and renaming the add-route-information to assoc-route-info, it doesn't seem like there's much added to having that private fn so I removed it. Is that cool?

Thanks for your feedback!

@weavejester

This comment has been minimized.

Owner

weavejester commented Jun 4, 2015

Precomputation just avoids creating a new vector each time. So currently you've written:

 (defn make-route [method path handler]
   (if-method method
     (if-route path
       (wrap-route-middleware
         (fn [request]
           (let [route-info [(or method :any) (str path)]]
             (response/render (handler (assoc request :route route-info))
                              request)))))))

But we can take the route-info binding out to the top level instead:

 (defn make-route [method path handler]
   (let [route-info [(or method :any) (str path)]]
     (if-method method
       (if-route path
         (wrap-route-middleware
           (fn [request]
             (let [request (assoc request :route route-info)]
               (response/render (handler request) request))))))))

Now the route-info vector is only calculated once, when the route is first constructed. Even though building the route-info isn't likely to take long, if we can save a little time and memory just by moving the let binding around, we should do it.

@weavejester

This comment has been minimized.

Owner

weavejester commented Jun 4, 2015

Removing the private function was the right call, I think. Could you also add a test to check that ANY routes work okay as well? They're a special case, so we should ensure they work.

@Christopher-Bui

This comment has been minimized.

Contributor

Christopher-Bui commented Jul 1, 2015

@weavejester

This comment has been minimized.

Owner

weavejester commented Jul 1, 2015

Thanks for the ping. This should be okay, but I think that :compojure/route might be a better key, as :route is a little generic. Make that change and I'll merge :)

@Christopher-Bui

This comment has been minimized.

Contributor

Christopher-Bui commented Jul 5, 2015

@weavejester good?

@weavejester

This comment has been minimized.

Owner

weavejester commented Jul 5, 2015

That looks fine. Now you just need to squash your commits.

@Christopher-Bui Christopher-Bui force-pushed the Christopher-Bui:route-information branch from 7aeb8ea to d443944 Jul 5, 2015

@Christopher-Bui

This comment has been minimized.

Contributor

Christopher-Bui commented Jul 5, 2015

Squashed.

@weavejester

This comment has been minimized.

Owner

weavejester commented Jul 5, 2015

Thanks! Can you remove the ending "." from the commit summary? Then I'll merge.

@Christopher-Bui Christopher-Bui force-pushed the Christopher-Bui:route-information branch from d443944 to fdef421 Jul 5, 2015

@Christopher-Bui

This comment has been minimized.

Contributor

Christopher-Bui commented Jul 5, 2015

Done!

weavejester added a commit that referenced this pull request Jul 5, 2015

Merge pull request #140 from Christopher-Bui/route-information
Add route information to request

@weavejester weavejester merged commit fad10ea into weavejester:master Jul 5, 2015

1 check passed

continuous-integration/travis-ci/pr The Travis CI build passed
Details
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment