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.

Sign up for GitHub

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

Jump to bottom

src: clarify OptionEnvvarSettings member names #45057

Merged
merged 1 commit into from
Oct 27, 2022
Merged

src: clarify OptionEnvvarSettings member names #45057

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

RaisinTen
RaisinTen approved these changes Oct 20, 2022
View reviewed changes
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
joyeecheung
joyeecheung approved these changes Oct 24, 2022
View reviewed changes
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.

@legendecas
src: clarify OptionEnvvarSettings member names
cb039c2 
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.
RafaelGSS
RafaelGSS approved these changes Oct 24, 2022
View reviewed changes
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

CI: https://ci.nodejs.org/job/node-test-pull-request/47504/

@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
@legendecas @RafaelGSS
src: clarify OptionEnvvarSettings member names
3657f6a 
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 <[email protected]>
Reviewed-By: Darshan Sen <[email protected]>
Reviewed-By: Joyee Cheung <[email protected]>
Reviewed-By: Rafael Gonzaga <[email protected]>
Reviewed-By: Anna Henningsen <[email protected]>
@RafaelGSS RafaelGSS mentioned this pull request Nov 1, 2022
Merged
RafaelGSS pushed a commit that referenced this pull request Nov 10, 2022
@legendecas @RafaelGSS
src: clarify OptionEnvvarSettings member names
243c141 
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 <[email protected]>
Reviewed-By: Darshan Sen <[email protected]>
Reviewed-By: Joyee Cheung <[email protected]>
Reviewed-By: Rafael Gonzaga <[email protected]>
Reviewed-By: Anna Henningsen <[email protected]>
@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
@legendecas
src: clarify OptionEnvvarSettings member names
0585746 
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 <[email protected]>
Reviewed-By: Darshan Sen <[email protected]>
Reviewed-By: Joyee Cheung <[email protected]>
Reviewed-By: Rafael Gonzaga <[email protected]>
Reviewed-By: Anna Henningsen <[email protected]>
@legendecas legendecas mentioned this pull request Dec 28, 2022
Closed
@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
This was referenced Jan 24, 2023
Merged
Merged
@LiviaMedeiros LiviaMedeiros mentioned this pull request Apr 6, 2023
Merged
legendecas added a commit to legendecas/node that referenced this pull request May 18, 2023
@legendecas
src: clarify OptionEnvvarSettings member names
37d7f81 
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 <[email protected]>
Reviewed-By: Darshan Sen <[email protected]>
Reviewed-By: Joyee Cheung <[email protected]>
Reviewed-By: Rafael Gonzaga <[email protected]>
Reviewed-By: Anna Henningsen <[email protected]>
danielleadams pushed a commit that referenced this pull request May 29, 2023
@legendecas @danielleadams
src: clarify OptionEnvvarSettings member names
0d5e324 
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 <[email protected]>
Reviewed-By: Darshan Sen <[email protected]>
Reviewed-By: Joyee Cheung <[email protected]>
Reviewed-By: Rafael Gonzaga <[email protected]>
Reviewed-By: Anna Henningsen <[email protected]>
@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
@danielleadams danielleadams mentioned this pull request Jul 10, 2023
Merged
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@RaisinTen RaisinTen RaisinTen approved these changes

@RafaelGSS RafaelGSS RafaelGSS approved these changes

@joyeecheung joyeecheung joyeecheung approved these changes

@addaleax addaleax addaleax approved these changes

@jasnell jasnell jasnell approved these changes

Assignees
No one assigned
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
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
10 participants
@legendecas @nodejs-github-bot @targos @danielleadams @jasnell @addaleax @joyeecheung @RafaelGSS @RaisinTen @juanarbol