-
Notifications
You must be signed in to change notification settings - Fork 29.6k
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
src: clarify OptionEnvvarSettings member names #45057
Conversation
a5806ba
to
1563488
Compare
Review requested:
|
@nodejs/cpp-reviewers |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The current name sounds better but one comment
f1ea7e2
to
8dfba84
Compare
8dfba84
to
ef5ac63
Compare
It came to me that embedders can actually parse the args from environment variables other than NODE_OPTIONS with the embedder API |
The term `Environment` in `kAllowedInEnvironment` and `kDisallowedInEnvironment` has nothing to do with the commonly used term `node::Environment`. Rename the member to `kAllowedInEnvvar` and `kDisallowedInEnvvar` respectively to reflect they are options of `OptionEnvvarSettings`. As `OptionEnvvarSettings` is part of the public embedder APIs, the legacy names are still preserved.
ef5ac63
to
cb039c2
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM.
Landed in 68a3ce5 |
The term `Environment` in `kAllowedInEnvironment` and `kDisallowedInEnvironment` has nothing to do with the commonly used term `node::Environment`. Rename the member to `kAllowedInEnvvar` and `kDisallowedInEnvvar` respectively to reflect they are options of `OptionEnvvarSettings`. As `OptionEnvvarSettings` is part of the public embedder APIs, the legacy names are still preserved. PR-URL: #45057 Reviewed-By: James M Snell <jasnell@gmail.com> Reviewed-By: Darshan Sen <raisinten@gmail.com> Reviewed-By: Joyee Cheung <joyeec9h3@gmail.com> Reviewed-By: Rafael Gonzaga <rafael.nunu@hotmail.com> Reviewed-By: Anna Henningsen <anna@addaleax.net>
The term `Environment` in `kAllowedInEnvironment` and `kDisallowedInEnvironment` has nothing to do with the commonly used term `node::Environment`. Rename the member to `kAllowedInEnvvar` and `kDisallowedInEnvvar` respectively to reflect they are options of `OptionEnvvarSettings`. As `OptionEnvvarSettings` is part of the public embedder APIs, the legacy names are still preserved. PR-URL: #45057 Reviewed-By: James M Snell <jasnell@gmail.com> Reviewed-By: Darshan Sen <raisinten@gmail.com> Reviewed-By: Joyee Cheung <joyeec9h3@gmail.com> Reviewed-By: Rafael Gonzaga <rafael.nunu@hotmail.com> Reviewed-By: Anna Henningsen <anna@addaleax.net>
@legendecas this broke the build when landing in v18.x. Do you mind creating a backport? Thank you. |
The term `Environment` in `kAllowedInEnvironment` and `kDisallowedInEnvironment` has nothing to do with the commonly used term `node::Environment`. Rename the member to `kAllowedInEnvvar` and `kDisallowedInEnvvar` respectively to reflect they are options of `OptionEnvvarSettings`. As `OptionEnvvarSettings` is part of the public embedder APIs, the legacy names are still preserved. PR-URL: nodejs#45057 Reviewed-By: James M Snell <jasnell@gmail.com> Reviewed-By: Darshan Sen <raisinten@gmail.com> Reviewed-By: Joyee Cheung <joyeec9h3@gmail.com> Reviewed-By: Rafael Gonzaga <rafael.nunu@hotmail.com> Reviewed-By: Anna Henningsen <anna@addaleax.net>
@danielleadams created #45994. |
The term `Environment` in `kAllowedInEnvironment` and `kDisallowedInEnvironment` has nothing to do with the commonly used term `node::Environment`. Rename the member to `kAllowedInEnvvar` and `kDisallowedInEnvvar` respectively to reflect they are options of `OptionEnvvarSettings`. As `OptionEnvvarSettings` is part of the public embedder APIs, the legacy names are still preserved. PR-URL: nodejs#45057 Reviewed-By: James M Snell <jasnell@gmail.com> Reviewed-By: Darshan Sen <raisinten@gmail.com> Reviewed-By: Joyee Cheung <joyeec9h3@gmail.com> Reviewed-By: Rafael Gonzaga <rafael.nunu@hotmail.com> Reviewed-By: Anna Henningsen <anna@addaleax.net>
The term `Environment` in `kAllowedInEnvironment` and `kDisallowedInEnvironment` has nothing to do with the commonly used term `node::Environment`. Rename the member to `kAllowedInEnvvar` and `kDisallowedInEnvvar` respectively to reflect they are options of `OptionEnvvarSettings`. As `OptionEnvvarSettings` is part of the public embedder APIs, the legacy names are still preserved. PR-URL: #45057 Backport-PR-URL: #45994 Reviewed-By: James M Snell <jasnell@gmail.com> Reviewed-By: Darshan Sen <raisinten@gmail.com> Reviewed-By: Joyee Cheung <joyeec9h3@gmail.com> Reviewed-By: Rafael Gonzaga <rafael.nunu@hotmail.com> Reviewed-By: Anna Henningsen <anna@addaleax.net>
The term
Environment
inkAllowedInEnvironment
andkDisallowedInEnvironment
has nothing to do with the commonly usedterm
node::Environment
. Rename the member tokAllowedInEnvvar
andkDisallowedInEnvvar
respectively to reflect they are options ofOptionEnvvarSettings
.As
OptionEnvvarSettings
is part of the public embedder APIs, thelegacy names are still preserved.