Adding `Route.prototype.realize`. #134

Merged
merged 1 commit into from May 4, 2012

Conversation

Projects
None yet
2 participants
Contributor

domenic commented Apr 25, 2012

Gives you a route's URL with params replaced by a passed hash's values.

I'm about 90% sure it conforms to restify's route-matching rules, which seem to be that route params can only be path segments (i.e. sandwiched between /s), and have no restrictions on valid characters.

I think there might be better places in the API for this, but Route was pretty convenient. It is a bit strange though since it is really a URL-level operation, not a route-level one. (That is, method is entirely ignored.)

An alternative might be just restify.realizeUrl(url, params).

Also feel free to suggest a better name.

Contributor

mcavage commented Apr 26, 2012

Hey Domenic,

Yeah that's cool - a couple comments:

(1) I probably would just put this somewhere on the top-level object, since it feels like a helper to users, more than anything.
(2) This should probably hook into the sanitizePath bit (i.e., handle /////foo/////bar/////).
(3) Why have this even take a params object? Why not just return a new object with params, and leave it to the caller to merge with whatever else they already have?

Contributor

domenic commented Apr 26, 2012

  1. Sounds good.
  2. Oooh, tricky. Test case?
  3. I don't really understand this point. It returns a string, not an object... what would your proposed API look like?
Contributor

mcavage commented Apr 26, 2012

Haha - I'm a dolt - disregard (3) - I read your test case backwards :).

For (2) there's this internal function in request: https://github.com/mcavage/node-restify/blob/master/lib/request.js#L22-39

I'm pretty sure there are test cases for that - so what you probably need to do is pull that into something consumable by your new API (i.e., need a ./lib/utils.js, or some such).

Contributor

domenic commented Apr 26, 2012

OK so then the test case would be

restify.realizeUrl("/foo////bar///:baz", {baz: 'BAZ'}) === '/foo/bar/BAZ'

?

I am going to push further commits to this branch as I make them, but please do not merge :). When we're both happy with the end result I will squash them into one commit and open a new pull request.

Contributor

domenic commented May 1, 2012

OK, how does this look? Ready for squashing?

Contributor

mcavage commented May 3, 2012

Hi Domenic,

Sorry - the notification for this got lost somewhere in spam folders. Anyway - yes, that looks great. Do you want to squash, or should I take it as-is?

m

@domenic domenic Adding `restify.realizeUrl` for replacing URL params with values from…
… a hash.

Example: `restify.realizeUrl('/foo/:bar/:baz', {bar: "BAR", baz: "BAZ"}) === '/foo/BAR/BAZ'`

In the process factored out `sanitizePath` method from request.js into a new utils.js module.
9bb51af
Contributor

domenic commented May 4, 2012

HULK SQUASH!!! ;)

@mcavage mcavage added a commit that referenced this pull request May 4, 2012

@mcavage mcavage Merge pull request #134 from domenic/realize
Adding `Route.prototype.realize`.
1331bf6

@mcavage mcavage merged commit 1331bf6 into restify:master May 4, 2012

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment