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

src: clarify OptionEnvvarSettings member names #45057

Merged
merged 1 commit into from
Oct 27, 2022

Conversation

legendecas
Copy link
Member

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.

@nodejs-github-bot
Copy link
Collaborator

Review requested:

  • @nodejs/startup

@nodejs-github-bot nodejs-github-bot added c++ Issues and PRs that require attention from people who are familiar with C++. lib / src Issues and PRs related to general changes in the lib or src directory. needs-ci PRs that need a full CI run. labels Oct 18, 2022
@targos
Copy link
Member

targos commented Oct 20, 2022

@nodejs/cpp-reviewers

Copy link
Contributor

@RaisinTen RaisinTen left a 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

src/node.h Show resolved Hide resolved
src/node.h Outdated Show resolved Hide resolved
@legendecas
Copy link
Member Author

It came to me that embedders can actually parse the args from environment variables other than NODE_OPTIONS with the embedder API ProcessGlobalArgs. So I reverted the name to kAllowedInEnvvar and kDisallowedInEnvvar and documented these members and the API ProcessGlobalArgs.

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.
Copy link
Member

@RafaelGSS RafaelGSS left a comment

Choose a reason for hiding this comment

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

LGTM.

@legendecas legendecas added commit-queue Add this label to land a pull request using GitHub Actions. request-ci Add this label to start a Jenkins CI on a PR. and removed commit-queue Add this label to land a pull request using GitHub Actions. labels Oct 26, 2022
@github-actions github-actions bot removed the request-ci Add this label to start a Jenkins CI on a PR. label Oct 26, 2022
@nodejs-github-bot
Copy link
Collaborator

@legendecas legendecas added the commit-queue Add this label to land a pull request using GitHub Actions. label Oct 27, 2022
@nodejs-github-bot nodejs-github-bot removed the commit-queue Add this label to land a pull request using GitHub Actions. label Oct 27, 2022
@nodejs-github-bot nodejs-github-bot merged commit 68a3ce5 into nodejs:main Oct 27, 2022
@nodejs-github-bot
Copy link
Collaborator

Landed in 68a3ce5

@legendecas legendecas deleted the env-options branch October 27, 2022 16:26
RafaelGSS pushed a commit that referenced this pull request Nov 1, 2022
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>
@RafaelGSS RafaelGSS mentioned this pull request Nov 1, 2022
RafaelGSS pushed a commit that referenced this pull request Nov 10, 2022
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>
@danielleadams
Copy link
Contributor

@legendecas this broke the build when landing in v18.x. Do you mind creating a backport? Thank you.

@danielleadams danielleadams added the backport-requested-v18.x PRs awaiting manual backport to the v18.x-staging branch. label Dec 27, 2022
legendecas added a commit to legendecas/node that referenced this pull request Dec 28, 2022
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>
@legendecas
Copy link
Member Author

@danielleadams created #45994.

@legendecas legendecas added backport-open-v18.x Indicate that the PR has an open backport. and removed backport-requested-v18.x PRs awaiting manual backport to the v18.x-staging branch. labels Dec 28, 2022
legendecas added a commit to legendecas/node that referenced this pull request May 18, 2023
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 pushed a commit that referenced this pull request May 29, 2023
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>
@juanarbol juanarbol added backported-to-v18.x PRs backported to the v18.x-staging branch. and removed backport-open-v18.x Indicate that the PR has an open backport. labels May 31, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
backported-to-v18.x PRs backported to the v18.x-staging branch. c++ Issues and PRs that require attention from people who are familiar with C++. lib / src Issues and PRs related to general changes in the lib or src directory. needs-ci PRs that need a full CI run.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

10 participants