Skip to content
This repository has been archived by the owner on Feb 28, 2024. It is now read-only.

Add a full-route key to the request map #1

Closed
wants to merge 2 commits into from

Conversation

liamchzh
Copy link

This provides access to the full route that contains the common prefix.
The full-route info is added to the ':compojure/full-route' key in the
request map.

@liamchzh
Copy link
Author

liamchzh commented Apr 25, 2022

Couldn't add backplane as the reviewer, so pint you folks here in the comment @circleci/backplane

I'd also provide more context and rationale when opening the PR to the real Compojre project. Input is welcome!

@liamchzh liamchzh requested a review from a team April 26, 2022 15:07
This provides access to the full route that contains the common prefix.
The full-route info is added to the ':compojure/full-route' key in the
request map.

Co-authored-by: Liam Chen <[email protected]>
Co-authored-by: Claire Alvis <[email protected]>
@liamchzh
Copy link
Author

I'd also provide more context and rationale when opening the PR to the real Compojre project.

We use :compojure/route to get the route info and add it to the attributes of our metrics and traces, but when compojure.core/context is used, we are not able to get the parameters that are not instantiated. This PR adds :compojure/route key that serves similarly as the existing :compojure/route does - the only difference is that the new key has a common prefix.

(clout/route-compile (str route# ":__path-info") ~re-context)
(if (string? route#)
{:context-route route#}
{}))))))
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

in the PR, you should mention that the with-meta is our attempt to not change the type of make-context, as it is public (albeit not documented). we could make a more direct map-based implementation if backwards compatibility of make-context is not important

also not entirely sure what the full syntax is of context, so it's possible we're missing a case

Copy link

@emilywoods emilywoods left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

After talking through these changes, the pr looks good to me

This approach is simpler, pass the path down, rather than
attaching it to the meta.
@liamchzh
Copy link
Author

weavejester#212 has been merged!

@liamchzh liamchzh closed this Jun 30, 2022
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants