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

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

Merged
merged 2 commits into from
Jun 14, 2018
Merged

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

merged 2 commits into from
Jun 14, 2018

Conversation

vsemozhetbyt
Copy link
Contributor

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

$ export NODE_OPTIONS='--trace-warnings --pending-deprecation'
```bash
$ export NODE_OPTIONS=--trace-warnings
$ export NODE_PENDING_DEPRECATION=1
Copy link
Contributor Author

Choose a reason for hiding this comment

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

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

Copy link
Member

Choose a reason for hiding this comment

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

Copy link
Contributor Author

@vsemozhetbyt vsemozhetbyt Jun 9, 2018

Choose a reason for hiding this comment

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

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

Copy link
Member

Choose a reason for hiding this comment

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

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.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

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

Node options that are allowed are: ...

Copy link
Member

Choose a reason for hiding this comment

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

I think it is simply not listed there.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

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.

Copy link
Member

Choose a reason for hiding this comment

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

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Thank you. I will revert this change.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

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


```console
$ export NODE_OPTIONS='--trace-warnings --pending-deprecation'
```bash
Copy link
Contributor Author

Choose a reason for hiding this comment

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

console tag seems to produce weird rendering:

1

@vsemozhetbyt
Copy link
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 vsemozhetbyt added content Issues/pr concerning content pr labels Jun 3, 2018
@vsemozhetbyt
Copy link
Contributor Author

cc @nodejs/documentation @nodejs/buffer

@lpinca
Copy link
Member

lpinca commented Jun 9, 2018

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
Copy link
Contributor Author

vsemozhetbyt commented Jun 9, 2018

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

@fhinkel Is this an appropriate fix?

@fhemberger fhemberger merged commit 692b8db into nodejs:master Jun 14, 2018
@fhemberger
Copy link
Contributor

@vsemozhetbyt Thanks!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
content Issues/pr concerning content
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants