FazBrowse GitHub Viewer | Trending |
URL:
| Home
Tools: [Download Repo ZIP]   [Original HTTPS Page]

guides: fix nits in buffer-constructor-deprecation by vsemozhetbyt · Pull Request #1679 · nodejs/nodejs.org · GitHub

/ nodejs.org Public

guides: fix nits in buffer-constructor-deprecation - #1679

Merged
fhemberger merged 2 commits into
nodejs:masterfrom
vsemozhetbyt:doc-guide-buffer-constructor-deprecation
Jun 14, 2018
ConversationCommits (2)ChecksFiles changed
Merged

guides: fix nits in buffer-constructor-deprecation#1679
fhemberger merged 2 commits into
nodejs:masterfrom
vsemozhetbyt:doc-guide-buffer-constructor-deprecation

Conversation

vsemozhetbyt commented Jun 3, 2018

Copy link
Copy Markdown
Contributor

This PR unify some formatting differences across the doc and fix some other nits.

vsemozhetbyt commented Jun 3, 2018
View reviewed changes
Comment thread locale/en/docs/guides/buffer-constructor-deprecation.md Outdated

vsemozhetbyt Jun 3, 2018

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

Choose a reason Spam Abuse Off Topic Outdated Duplicate Resolved Low Quality

It seems we have not --pending-deprecation in NODE_OPTIONS.

lpinca Jun 9, 2018

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Choose a reason Spam Abuse Off Topic Outdated Duplicate Resolved Low Quality

@vsemozhetbyt https://nodejs.org/download/nightly/v11.0.0-nightly20180530c1a87a30e7/docs/api/cli.html#cli_pending_deprecation

vsemozhetbyt Jun 9, 2018 •
edited
Loading

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

Choose a reason Spam Abuse Off Topic Outdated Duplicate Resolved Low Quality

Yes, we have it as a separate cli key, but not as a NODE_OPTIONS env variable part option.

lpinca Jun 9, 2018

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Choose a reason Spam Abuse Off Topic Outdated Duplicate Resolved Low Quality

I'm not sure I follow, --pending-deprecation is a CLI option right? If so it should work when used via NODE_OPTIONS which allows to specify CLI options in the environment via a space-separated list.

vsemozhetbyt Jun 9, 2018

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

Choose a reason Spam Abuse Off Topic Outdated Duplicate Resolved Low Quality

It seems we have a whitelist of cli keys that are allowed in NODE_OPTIONS:

Node options that are allowed are: ...

lpinca Jun 9, 2018

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Choose a reason Spam Abuse Off Topic Outdated Duplicate Resolved Low Quality

I think it is simply not listed there.

vsemozhetbyt Jun 9, 2018

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

Choose a reason Spam Abuse Off Topic Outdated Duplicate Resolved Low Quality

Node.js 10 doc has not it as well:

https://nodejs.org/dist/latest-v10.x/docs/api/cli.html#cli_node_options_options

If it does work, maybe the API docs need updating.

lpinca Jun 9, 2018

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Choose a reason Spam Abuse Off Topic Outdated Duplicate Resolved Low Quality

Yes it seems outdated, see https://github.com/nodejs/node/blob/master/src/node.cc#L3156-L3198

vsemozhetbyt Jun 9, 2018

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

Choose a reason Spam Abuse Off Topic Outdated Duplicate Resolved Low Quality

Thank you. I will revert this change.

vsemozhetbyt Jun 9, 2018

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

Choose a reason Spam Abuse Off Topic Outdated Duplicate Resolved Low Quality

PR to update the API doc: nodejs/node#21229

vsemozhetbyt commented Jun 3, 2018
View reviewed changes
Comment thread locale/en/docs/guides/buffer-constructor-deprecation.md Outdated

vsemozhetbyt Jun 3, 2018

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

Choose a reason Spam Abuse Off Topic Outdated Duplicate Resolved Low Quality

console tag seems to produce weird rendering:

vsemozhetbyt commented Jun 3, 2018

Copy link
Copy Markdown
Contributor Author

I am a bit confused by this fragment in the example code. Are the condition check and error message in some contradiction?

vsemozhetbyt added content Issues/pr concerning content pr labels Jun 3, 2018

vsemozhetbyt commented Jun 8, 2018

Copy link
Copy Markdown
Contributor Author

cc @nodejs/documentation @nodejs/buffer

lpinca commented Jun 9, 2018

Copy link
Copy Markdown
Member

I am a bit confused by this fragment in the example code. Are the condition check and error message in some contradiction?

Yes, I think so the message should be 'The "size" argument must not be of type number.'

vsemozhetbyt commented Jun 9, 2018 •
edited
Loading

Copy link
Copy Markdown
Contributor Author

Yes, I think so the message should be 'The "size" argument must not be of type number.'

@fhinkel Is this an appropriate fix?

fhemberger merged commit 692b8db into nodejs:master Jun 14, 2018

fhemberger commented Jun 14, 2018

Copy link
Copy Markdown
Contributor

@vsemozhetbyt Thanks!

vsemozhetbyt deleted the doc-guide-buffer-constructor-deprecation branch June 14, 2018 15:39
This was referenced Oct 3, 2021
Open
Open
Merged
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters. Learn more about bidirectional Unicode characters
Show hidden characters
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

lpinca lpinca left review comments

Assignees

No one assigned

Labels

content Issues/pr concerning content

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

3 participants

Back | FazBrowse Home | New Git URL