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

http.ClientRequest doc update #5719

Closed
wants to merge 1 commit into from
Closed

Conversation

mrgleba
Copy link

@mrgleba mrgleba commented Mar 15, 2016

Pull request supplying the missing methods:
setHeader / getheader / removeHeader
addTrailers

@Fishrock123 Fishrock123 added http Issues or PRs related to the http subsystem. doc Issues and PRs related to the documentations. labels Mar 15, 2016
@benjamingr
Copy link
Member

cc @nodejs/http @nodejs/documentation

@jasnell
Copy link
Member

jasnell commented Mar 15, 2016

LGTM

@benjamingr
Copy link
Member

Generally LGTM, I'd like to understand how such a gap has formed and how to identify the gaps in the future - would rather we wait with the merge until someone from /http or someone with historic reference can shed some light.


```js
var request = http.request({
method: 'POST',
Copy link
Contributor

Choose a reason for hiding this comment

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

We normally use two spaces for indentation.

@mrgleba
Copy link
Author

mrgleba commented Mar 21, 2016

I've updated the formatting.

@eljefedelrodeodeljefe
Copy link
Contributor

LGTM. I think squashing the three commits, would be cool.

@mrgleba
Copy link
Author

mrgleba commented Mar 21, 2016

I'm rather new to git (we use mercurial in shop).
Tried squashing following the instructions:
http://blog.steveklabnik.com/posts/2012-11-08-how-to-squash-commits-in-a-github-pull-request

I can't tell if I;ve done ok.

Please verify.

@benjamingr
Copy link
Member

If it's too much trouble you can revert the squash and we'll do it for you on our end (the squishing).

I think that what happened is that you pulled master on top of your changes instead of rebasing your changes on top of master.

@mrgleba mrgleba force-pushed the master branch 2 times, most recently from e0489d6 to cb51d12 Compare March 21, 2016 12:10
@mrgleba
Copy link
Author

mrgleba commented Mar 21, 2016

OK. I think I've got it right this time :)

@eljefedelrodeodeljefe
Copy link
Contributor

Almost perfect :) Since I cannot land this anyway, I don't mind too much, but if you can It'd be appreciated that the commit was named during rebase (see contributor guide). git rebase -i HEAD~1 and just editing the commit message in your text editor, should be working. Something like doc, http: add .addTrailers() description would be very descriptive. git push --force afterwards.

Thanks for the effort @mrgleba and welcome to the (horrible) wonders of git. 👯

@mrgleba
Copy link
Author

mrgleba commented Mar 21, 2016

Done.

@jasnell
Copy link
Member

jasnell commented Mar 21, 2016

/cc @indutny

@jasnell
Copy link
Member

jasnell commented May 17, 2016

Ping @indutny
@mrgleba ... can you please rebase this :-)

@indutny
Copy link
Member

indutny commented May 17, 2016

Yikes, missed this. Looking through it.

@indutny
Copy link
Member

indutny commented May 17, 2016

Hm... perhaps, we should just mention that it inherits from the OutgoingMessage? Seems like this way we will avoid the duplication.

@jasnell
Copy link
Member

jasnell commented Jan 6, 2017

Ping... should this still happen?

@jasnell jasnell added the stalled Issues and PRs that are stalled. label Jan 6, 2017
@jasnell
Copy link
Member

jasnell commented Feb 28, 2017

Closing given the lack of forward progress

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
doc Issues and PRs related to the documentations. http Issues or PRs related to the http subsystem. stalled Issues and PRs that are stalled.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

7 participants