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

doc: add added: information for stream #7287

Closed
wants to merge 12 commits into from
Closed

doc: add added: information for stream #7287

wants to merge 12 commits into from

Conversation

italoacasas
Copy link
Contributor

Checklist
  • documentation is changed or added
  • the commit message follows commit guidelines
Affected core subsystem(s)
  • doc
Description of change

Add added: information for stream

@nodejs-github-bot nodejs-github-bot added the doc Issues and PRs related to the documentations. label Jun 13, 2016
@italoacasas italoacasas mentioned this pull request Jun 13, 2016
Closed
2 tasks
@sam-github sam-github mentioned this pull request Jun 13, 2016
Closed
36 tasks
@mscdex mscdex added the stream Issues and PRs related to the stream subsystem. label Jun 13, 2016
@claudiorodriguez
Copy link
Contributor

Some minor things:

Other than that, the actual content of the PR LGTM
Thanks!

addaleax
addaleax reviewed Jun 15, 2016
View reviewed changes
doc/api/stream.md
@@ -205,10 +205,16 @@ myStream.end('done writing data');
```

#### Class: stream.Writable
<!-- YAML
added: v0.3.0
-->
Copy link
Member

Choose a reason for hiding this comment

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

0.9.4

@italoacasas italoacasas mentioned this pull request Jun 19, 2016
Closed
@italoacasas
Copy link
Contributor Author

Writable Streams section is done.

@italoacasas
Copy link
Contributor Author

italoacasas commented Jun 21, 2016 •
edited
Loading

Readable Streams section is done

addaleax
addaleax reviewed Jun 23, 2016
View reviewed changes
doc/api/stream.md
@@ -325,6 +349,9 @@ implementations that implement the `writable.\_writev()` method can perform
buffered writes in a more optimized manner.

##### writable.end([chunk][, encoding][, callback])
<!-- YAML
added: v0.9.9
-->
Copy link
Member

Choose a reason for hiding this comment

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

$ nvm use 0.9.4
Now using node v0.9.4
$ node
> typeof require('stream').Writable.prototype.end
'function'

Copy link
Contributor Author

@italoacasas italoacasas Jun 23, 2016 •
edited
Loading

Choose a reason for hiding this comment

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

Yeah, but in 0.9.4 that method doesn't accept a callback

a9c4a20

Copy link
Member

Choose a reason for hiding this comment

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

@italoacasas I’d still prefer to go with the version it was added in; adding some sort of changelog for individual methods is definitely planned.

Copy link
Contributor Author

@italoacasas italoacasas Jun 23, 2016 •
edited
Loading

Choose a reason for hiding this comment

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

ok

@addaleax
Copy link
Member

addaleax commented Jun 23, 2016 •
edited
Loading

LGTM with .end() nit addressed.

@addaleax
Copy link
Member

@italoacasas ping

@italoacasas
Copy link
Contributor Author

sorry for the late response. I was on vacation, I'll finish this PR this week.

@italoacasas
Copy link
Contributor Author

I'm back working on this.

@italoacasas
Copy link
Contributor Author

This should be ready, I don't find any other place where to add added

@addaleax
Copy link
Member

LGTM

@addaleax
Copy link
Member

addaleax commented Jul 17, 2016 •
edited
Loading

Landed in 06cfecd0325e301be866c7ff9c7544f1a1cdcbec c897d0b, thanks!

@addaleax addaleax closed this Jul 17, 2016
addaleax pushed a commit to addaleax/node that referenced this pull request Jul 17, 2016
@italoacasas @addaleax
doc: add added: information for stream
c897d0b 
Ref: nodejs#6578
PR-URL: nodejs#7287
Reviewed-By: Anna Henningsen <anna@addaleax.net>
Reviewed-By: Claudio Rodriguez <cjrodr@yahoo.com>
evanlucas pushed a commit that referenced this pull request Jul 19, 2016
@italoacasas @evanlucas
doc: add added: information for stream
f7d3af6 
Ref: #6578
PR-URL: #7287
Reviewed-By: Anna Henningsen <anna@addaleax.net>
Reviewed-By: Claudio Rodriguez <cjrodr@yahoo.com>
@evanlucas evanlucas mentioned this pull request Jul 19, 2016
Merged
evanlucas pushed a commit that referenced this pull request Jul 20, 2016
@italoacasas @evanlucas
doc: add added: information for stream
9847f84 
Ref: #6578
PR-URL: #7287
Reviewed-By: Anna Henningsen <anna@addaleax.net>
Reviewed-By: Claudio Rodriguez <cjrodr@yahoo.com>
@MylesBorins
Copy link
Contributor

this is not landing cleanly on v4.x. Please submit a backport if you would like to see it land

@italoacasas
Copy link
Contributor Author

I'm going to do it tonight

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
doc Issues and PRs related to the documentations. stream Issues and PRs related to the stream subsystem.
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
6 participants
@italoacasas @claudiorodriguez @addaleax @MylesBorins @mscdex @nodejs-github-bot