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

Use self.keepAlive instead of self.options.keepAlive in http agent #4407

Closed
wants to merge 1 commit into from

Conversation

dschenkelman
Copy link
Contributor

All other options are directly accessed through self. not self.options, so this looks like the odd one out.

All other options are directly accessed through `self.` not `self.options`, so this looks like the odd one out.
@jasnell jasnell added the http Issues or PRs related to the http subsystem. label Dec 24, 2015
@jasnell
Copy link
Member

jasnell commented Dec 24, 2015

LGTM
/cc @nodejs/http

@mscdex
Copy link
Contributor

mscdex commented Dec 24, 2015

LGTM

1 similar comment
@indutny
Copy link
Member

indutny commented Dec 24, 2015

LGTM

@cjihrig
Copy link
Contributor

cjihrig commented Dec 29, 2015

LGTM

jasnell pushed a commit that referenced this pull request Dec 30, 2015
In http.agent, all other options are directly accessed through
`self.` not `self.options`.

PR-URL: #4407
Reviewed-By: Brian White <mscdex@mscdex.net>
Reviewed-By: Fedor Indutny <fedor.indutny@gmail.com>
Reviewed-By: Colin Ihrig <cjihrig@gmail.com>
Reviewed-By: James M Snell <jasnell@gmail.com>
@jasnell
Copy link
Member

jasnell commented Dec 30, 2015

Landed in 8ac1ff7.
@dschenkelman ... just an fyi, I fixed up the commit log message to match our standard guidelines. The commit subject was overlong and did not include the module prefix; and the commit message itself needed linewrapping. Minor stuff but good to keep in mind for next time :-)

@jasnell jasnell closed this Dec 30, 2015
@dschenkelman
Copy link
Contributor Author

thanks @jasnell! will follow that pattern next time :)

@rvagg
Copy link
Member

rvagg commented Jan 4, 2016

Nice fix, thanks for this @dschenkelman! I believe this is your first commit to core, welcome on board! I hope you can find other places to contribute.

@dschenkelman dschenkelman deleted the patch-1 branch January 4, 2016 13:26
@dschenkelman
Copy link
Contributor Author

@rvagg thanks! Hopefully I will 😄

Fishrock123 pushed a commit to Fishrock123/node that referenced this pull request Jan 6, 2016
In http.agent, all other options are directly accessed through
`self.` not `self.options`.

PR-URL: nodejs#4407
Reviewed-By: Brian White <mscdex@mscdex.net>
Reviewed-By: Fedor Indutny <fedor.indutny@gmail.com>
Reviewed-By: Colin Ihrig <cjihrig@gmail.com>
Reviewed-By: James M Snell <jasnell@gmail.com>
MylesBorins pushed a commit that referenced this pull request Jan 13, 2016
In http.agent, all other options are directly accessed through
`self.` not `self.options`.

PR-URL: #4407
Reviewed-By: Brian White <mscdex@mscdex.net>
Reviewed-By: Fedor Indutny <fedor.indutny@gmail.com>
Reviewed-By: Colin Ihrig <cjihrig@gmail.com>
Reviewed-By: James M Snell <jasnell@gmail.com>
MylesBorins pushed a commit that referenced this pull request Jan 19, 2016
In http.agent, all other options are directly accessed through
`self.` not `self.options`.

PR-URL: #4407
Reviewed-By: Brian White <mscdex@mscdex.net>
Reviewed-By: Fedor Indutny <fedor.indutny@gmail.com>
Reviewed-By: Colin Ihrig <cjihrig@gmail.com>
Reviewed-By: James M Snell <jasnell@gmail.com>
@MylesBorins MylesBorins mentioned this pull request Jan 19, 2016
scovetta pushed a commit to scovetta/node that referenced this pull request Apr 2, 2016
In http.agent, all other options are directly accessed through
`self.` not `self.options`.

PR-URL: nodejs#4407
Reviewed-By: Brian White <mscdex@mscdex.net>
Reviewed-By: Fedor Indutny <fedor.indutny@gmail.com>
Reviewed-By: Colin Ihrig <cjihrig@gmail.com>
Reviewed-By: James M Snell <jasnell@gmail.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
http Issues or PRs related to the http subsystem.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

7 participants