Skip to content
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

Adding req.matchedVersion() #635

Closed
wants to merge 1 commit into from
Closed

Conversation

nathanpeck
Copy link
Contributor

When a route is defined as responding to multiple version numbers then if the client supplies a version range that it can accept such as accept-version: <2.2.0 it can be useful for the route to know exactly which version number from its array of supported versions matched the clients version specification.

req.version() already exposes the clients version string but it is often a range such as <2.2.0 or 2.*
req.matchedVersion() allows a route which matches versions 2.0.0, 2.1.0, and 2.2.0 to know that a client who specified accept-version: <2.2.0 reached the route because 2.1.0 matched the request header.

This pull request includes documentation, and a test.

@@ -203,6 +203,14 @@ Request.prototype.getVersion = function getVersion() {
};
Request.prototype.version = Request.prototype.getVersion;

Request.prototype.matchedVersion = function matchedVersion() {
if (this._matchedVersion !== undefined)
Copy link
Member

Choose a reason for hiding this comment

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

if (x) {

} else {

}

This new method makes it easier for routes to determine which version from their list of supported versions matched the accept-version header string.

Rebase added committed: Fixing stylistic issues with pull request
@nathanpeck
Copy link
Contributor Author

Sorry about that stylistic faux paux @gergelyke. I fixed the if/else format and squashed the stylistic change commit into the original.

@yunong
Copy link
Member

yunong commented Feb 25, 2015

Hey @nathanpeck , thanks so much for doing this. In order to merge, could you

  1. prefix the unit test name with 'gh Adding req.matchedVersion() #635'
  2. make sure make prepush runs clean.

@@ -367,6 +367,7 @@ Router.prototype.find = function find(req, res, callback) {
}
}

var maxV;
Copy link
Member

Choose a reason for hiding this comment

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

Can you move this up to the top of the function, with the rest of the variable declarations?

@micahr
Copy link
Member

micahr commented Jun 5, 2015

@nathanpeck Almost there, just a couple small changes (and merge conflict resolutions) and this is good to go!

@micahr
Copy link
Member

micahr commented Jun 20, 2015

Closing in favor of #842 which fixes the merge conflict and JSCS errors

@micahr micahr closed this Jun 20, 2015
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

4 participants