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: rename defaultEncoding option to encoding #14867

Closed
wants to merge 1 commit into from
Closed

doc: rename defaultEncoding option to encoding #14867

wants to merge 1 commit into from

Conversation

azasypkin
Copy link
Contributor

The documentation for fs.createWriteStream() references a
defaultEncoding as possible options property, but in reality
encoding property is expected and properly handled. This fix updates
the documentation to rename the defaultEncoding property to
encoding.

Fixes: #14611

Checklist
Affected core subsystem(s)

doc

The documentation for `fs.createWriteStream()` references a
`defaultEncoding` as possible options property, but in reality
`encoding` property is expected and properly handled. This fix updates
the documentation to rename the `defaultEncoding` property to
`encoding`.

Fixes: #14611
@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. labels Aug 16, 2017
@refack
Copy link
Contributor

refack commented Aug 16, 2017

Refs:

node/lib/fs.js

Lines 2081 to 2082 in 2249234

if (options.encoding)
this.setDefaultEncoding(options.encoding);

node/lib/fs.js

Lines 79 to 80 in 2249234

if (options.encoding !== 'buffer')
assertEncoding(options.encoding);

@refack
Copy link
Contributor

refack commented Aug 16, 2017

Welcome @azasypkin, and thank you for the contribution 🥇
What the CONTRIBUTION.md doesn't mention is that documentation changes can get tricky. While core changes can be auto-tested, doc changes need human validation. So don't be dismayed if this PR get allot of attention 😉

@lance
Copy link
Member

lance commented Aug 17, 2017

@refack @lpinca looking at this I wondered why things still seem to work with options.defaultEncoding. It's because of this undocumented gem:

this.defaultEncoding = options.defaultEncoding || 'utf8';

When fs.createWritableStream() creates the new stream.Writable it's just passing along the user provided options.defaultEncoding (if there was one).

The Stream.Writable docs don't mention this as a possible option. So I guess this raises the question whether this also should be corrected. And maybe whether it should be left as-is for fs.createWritableStream.

refack: edited formatting to embed reffed line

@refack
Copy link
Contributor

refack commented Aug 17, 2017

The Stream.Writable docs don't mention this as a possible option. So I guess this raises the question whether this also should be corrected. And maybe whether it should be left as-is for fs.createWritableStream.

IMHO the docs for Stream.Writable should be updated as well (here, or in the separate PR is up to @azasypkin )
But IMHO the fs.createWritableStream docs should be changed since it leads to surprises - #14611

@refack
Copy link
Contributor

refack commented Aug 17, 2017

/cc @nodejs/streams @nodejs/documentation

@azasypkin
Copy link
Contributor Author

IMHO the docs for Stream.Writable should be updated as well (here, or in the separate PR is up to @azasypkin )

I think it makes sense to include this little change in this PR as well, I'll add it.

@mcollina
Copy link
Member

@refack @azasypkin I would not add that change here. Ideally, I will remove defaultEncoding  from stream.Writable and stream.Readable completely, as they are concern of just crypto.

Copy link
Member

@mcollina mcollina left a comment

Choose a reason for hiding this comment

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

LGTM

@azasypkin
Copy link
Contributor Author

@refack @azasypkin I would not add that change here. Ideally, I will remove defaultEncoding from stream.Writable and stream.Readable completely, as they are concern of just crypto.

Okay, it's even better :)

@refack
Copy link
Contributor

refack commented Aug 20, 2017

Landed in e96c11c

@refack refack closed this Aug 20, 2017
refack pushed a commit that referenced this pull request Aug 20, 2017
The documentation for `fs.createWriteStream()` references a
`defaultEncoding` as possible options property, but in reality
`encoding` property is expected and properly handled. This fix updates
the documentation to rename the `defaultEncoding` property to
`encoding`.

PR-URL: #14867
Fixes: #14611
Reviewed-By: Luigi Pinca <[email protected]>
Reviewed-By: Refael Ackermann <[email protected]>
Reviewed-By: Matteo Collina <[email protected]>
Reviewed-By: James M Snell <[email protected]>
@refack
Copy link
Contributor

refack commented Aug 20, 2017

@azasypkin congratulation on GitHub promoting you from:
image
to:
image
🍾
Hope to see you contribute again!

@azasypkin azasypkin deleted the issue-14611-write-stream-encoding-doc branch August 21, 2017 06:17
@azasypkin
Copy link
Contributor Author

@azasypkin congratulation on GitHub promoting you from:

Yay, thanks!

MylesBorins pushed a commit that referenced this pull request Sep 10, 2017
The documentation for `fs.createWriteStream()` references a
`defaultEncoding` as possible options property, but in reality
`encoding` property is expected and properly handled. This fix updates
the documentation to rename the `defaultEncoding` property to
`encoding`.

PR-URL: #14867
Fixes: #14611
Reviewed-By: Luigi Pinca <[email protected]>
Reviewed-By: Refael Ackermann <[email protected]>
Reviewed-By: Matteo Collina <[email protected]>
Reviewed-By: James M Snell <[email protected]>
@MylesBorins MylesBorins mentioned this pull request Sep 10, 2017
MylesBorins pushed a commit that referenced this pull request Sep 12, 2017
The documentation for `fs.createWriteStream()` references a
`defaultEncoding` as possible options property, but in reality
`encoding` property is expected and properly handled. This fix updates
the documentation to rename the `defaultEncoding` property to
`encoding`.

PR-URL: #14867
Fixes: #14611
Reviewed-By: Luigi Pinca <[email protected]>
Reviewed-By: Refael Ackermann <[email protected]>
Reviewed-By: Matteo Collina <[email protected]>
Reviewed-By: James M Snell <[email protected]>
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.

Documentation should mention encoding instead of defaultEncoding as options property for fs.createWriteStream
8 participants