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

doc: fix 'close' event descriptions for fs.ReadStream and fs.WriteStream #15790

Closed
wants to merge 1 commit into from
Closed

doc: fix 'close' event descriptions for fs.ReadStream and fs.WriteStream #15790

wants to merge 1 commit into from

Conversation

JamesMGreene
Copy link
Contributor

@JamesMGreene JamesMGreene commented Oct 6, 2017

Checklist
Affected core subsystem(s)

doc


I tried to follow the commit message guidelines but I couldn't think any of good message that was 50 characters or less. If anyone has recommendations, I'd be happy to update it.

Situation: These changes seem to already be in place within the docs for 4.x and 8.x but are somehow missing from 6.x. Seemed like an easy but necessary fix when I happened upon it in the docs the other day. 👍

@nodejs-github-bot nodejs-github-bot added doc Issues and PRs related to the documentations. fs Issues and PRs related to the fs subsystem / file system. v6.x labels Oct 6, 2017
@@ -182,7 +182,8 @@ added: v0.1.93

* `fd` {integer} Integer file descriptor used by the ReadStream.

Emitted when the ReadStream's file is opened.
Emitted when the `ReadStream`'s underlying file descriptor has been closed
using the `fs.close()` method.
Copy link
Contributor

Choose a reason for hiding this comment

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

I think this last part can/should be omitted. Just end on 'has been closed.'

Copy link
Contributor Author

@JamesMGreene JamesMGreene Oct 6, 2017

Choose a reason for hiding this comment

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

I fully agree but I copied that markdown verbatim from the v4.x and v8.x docs. As such, I'd suggest either changing it here but also in those spots (or at least v8.x) as well, or else leaving it intact here.

But yeah, that last bit seems unnecessary and, honestly, a bit awkward.

Copy link
Contributor Author

@JamesMGreene JamesMGreene Oct 6, 2017

Choose a reason for hiding this comment

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

How is the upstreaming of commits to other version branches typically handled for this repo? For example, if I changed it here in v6.x, would I submit separate PRs for all of the v7.x, v8.x, v9.x, and master branches, or do changes to some of them get upstream/merged into others?

Copy link
Contributor

Choose a reason for hiding this comment

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

Make the change in master and it will be backported at some point.

Copy link
Contributor Author

@JamesMGreene JamesMGreene Oct 6, 2017

Choose a reason for hiding this comment

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

@mscdex How far do you backport? Should I rebase this PR to be targeting master instead of directly to v6.x, or this PR plus another PR targeting master?

Copy link
Contributor

Choose a reason for hiding this comment

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

Well this particular change was made in other branches inadvertently(?) in #11331 it seems. So this PR is fine for now for consistency.

If you want to submit another PR to remove the last portion of this sentence, you'd file a PR against master and it will automatically trickle down to the versioned branches by releasers.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Sounds good. 👍

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Followup work submitted as PR #15800.

@@ -296,7 +297,8 @@ added: v0.1.93

* `fd` {integer} Integer file descriptor used by the WriteStream.

Emitted when the WriteStream's file is opened.
Emitted when the `WriteStream`'s underlying file descriptor has been closed
using the `fs.close()` method.
Copy link
Contributor

Choose a reason for hiding this comment

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

Ditto.

@joyeecheung
Copy link
Member

IIUC this has been superseded by #15800. Closing.

@JamesMGreene
Copy link
Contributor Author

JamesMGreene commented Oct 11, 2017

@joyeecheung: Only if someone makes sure that #15800 makes its way into the v6.x branch (which is the specific branch that this PR is targeting) as that branch has totally incorrect descriptions.

@joyeecheung
Copy link
Member

@JamesMGreene Usually people who do releases would take care of that, when there are conflicts they will ping you in the against-master PR about it and ask you to do a manual backport. If it cherry-picks into the release branch cleanly, it will just land. See How to Backport a Pull Request to a Release Line for details.

@MylesBorins
Copy link
Contributor

landed in v6.x-staging in 31e4f2b
thanks for the work!

@JamesMGreene
Copy link
Contributor Author

Thanks for that, @MylesBorins!

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. fs Issues and PRs related to the fs subsystem / file system.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants