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.

Sign up for GitHub

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

Jump to bottom

defaults: merge headers #946

Merged
merged 2 commits into from
Jul 7, 2014
Merged

defaults: merge headers #946

merged 2 commits into from
Jul 7, 2014

Conversation

aj0strow
Copy link
Contributor

As requested (#257) headers are merged into default headers. In the example below, the user agent isn't lost.

var req = request.defaults({
  headers: {
    'user-agent': 'Mozilla/5.0 (Macintosh; Intel Mac OS X 10_9_3)'
  }
})

req(url, { headers: { accept: 'application/json' } }, callback)

mikeal
mikeal reviewed Jun 26, 2014
View reviewed changes
index.js
@@ -65,10 +65,18 @@ request.initParams = initParams

request.defaults = function (options, requester) {
var def = function (method) {
var d = function (uri, opts, callback) {
return function (uri, opts, callback) {
Copy link
Member

Choose a reason for hiding this comment

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

as a matter of style, i don't like to have the return value definition extend to blow the return line.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

as a matter of style, i don't like 1-letter variables 😄 i'll change it tho

@aj0strow
Copy link
Contributor Author

aj0strow commented Jul 3, 2014

@mikeal thoughts?

@mikeal
Copy link
Member

mikeal commented Jul 7, 2014

I'm going to merge this. I'd like to do one release with all the current changes first though because the last time we touched this code it broke a lot of stuff.

mikeal added a commit that referenced this pull request Jul 7, 2014
@mikeal
Merge pull request #946 from aj0strow/merge-default-headers
8640de0 
defaults: merge headers
@mikeal mikeal merged commit 8640de0 into request:master Jul 7, 2014
@aj0strow aj0strow deleted the merge-default-headers branch July 7, 2014 18:35
@aj0strow
Copy link
Contributor Author

aj0strow commented Jul 7, 2014

Sure. Thanks for merging.

nylen pushed a commit to nylen/request that referenced this pull request Oct 17, 2014
@mikeal
Merge pull request request#946 from aj0strow/merge-default-headers
2a064c3 
defaults: merge headers
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers
No reviews
Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
2 participants
@aj0strow @mikeal