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

process: generate list of allowed env flags programmatically #22638

Closed
wants to merge 4 commits into from

Conversation

addaleax
Copy link
Member

Avoids having a separate, second source of truth on this matter.

The second diff is best viewed in whitespace-adjusted mode (append ?w=1 to GitHub URL).

Checklist
  • make -j4 test (UNIX), or vcbuild test (Windows) passes
  • commit message follows commit guidelines

@nodejs-github-bot nodejs-github-bot added the c++ Issues and PRs that require attention from people who are familiar with C++. label Aug 31, 2018
[...options].filter(([name, info]) =>
info.type === kV8Option &&
info.envVarSettings === kAllowedInEnvironment)
.map(([name]) => name);
Copy link
Member

@BridgeAR BridgeAR Sep 2, 2018

Choose a reason for hiding this comment

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

I would prefer to pass through filter options into getOptions instead of doing the filtering here.
Since it has a string option at the moment, it would likely be best to accept an object for the options instead.

Another alternative to the current way would be to only iterate once over the entries (this is also nicer to read for me):

const allowedV8EnvironmentFlags = [];
const allowedNodeEnvironmentFlags = [];
for (const [name, info] of options) {
  if (info.envVarSettings === kAllowedInEnvironment) {
    if (info.type === KV8Option) {
      allowedV8EnvironmentFlags.push(name);
    } else {
      allowedNodeEnvironmentFlags.push(name);
    }
  }
}

Copy link
Member Author

Choose a reason for hiding this comment

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

I’ve gone with the second option for now, but yes, ultimately it might be better to do this in C++ land.

for (const to of expansion) {
if (!to.startsWith('-')) continue;
if (aliases.get(to)) {
expansion.push(...aliases.get(to));
Copy link
Member

Choose a reason for hiding this comment

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

Nit: retrieving the value multiple times is not necessary.

@addaleax
Copy link
Member Author

addaleax commented Sep 5, 2018

@addaleax
Copy link
Member Author

addaleax commented Sep 5, 2018

New CI: https://ci.nodejs.org/job/node-test-pull-request/17028/ (:heavy_check_mark:)

@BridgeAR BridgeAR added the author ready PRs that have at least one approval, no pending requests for changes, and a CI started. label Sep 5, 2018
@addaleax
Copy link
Member Author

addaleax commented Sep 9, 2018

Landed in 995782c, d994e26

@addaleax addaleax closed this Sep 9, 2018
@addaleax addaleax deleted the allowed-in-env-flags branch September 9, 2018 14:23
addaleax added a commit that referenced this pull request Sep 9, 2018
Avoids having a separate, second source of truth on this matter.

PR-URL: #22638
Reviewed-By: James M Snell <jasnell@gmail.com>
Reviewed-By: Ruben Bridgewater <ruben@bridgewater.de>
addaleax added a commit that referenced this pull request Sep 9, 2018
PR-URL: #22638
Reviewed-By: James M Snell <jasnell@gmail.com>
Reviewed-By: Ruben Bridgewater <ruben@bridgewater.de>
@targos
Copy link
Member

targos commented Sep 10, 2018

This needs a backport PR to land on v10.x-staging

addaleax added a commit that referenced this pull request Sep 13, 2018
Avoids having a separate, second source of truth on this matter.

PR-URL: #22638
Reviewed-By: James M Snell <jasnell@gmail.com>
Reviewed-By: Ruben Bridgewater <ruben@bridgewater.de>
addaleax added a commit that referenced this pull request Sep 13, 2018
PR-URL: #22638
Reviewed-By: James M Snell <jasnell@gmail.com>
Reviewed-By: Ruben Bridgewater <ruben@bridgewater.de>
targos pushed a commit that referenced this pull request Sep 15, 2018
Avoids having a separate, second source of truth on this matter.

Backport-PR-URL: #22847
PR-URL: #22638
Reviewed-By: James M Snell <jasnell@gmail.com>
Reviewed-By: Ruben Bridgewater <ruben@bridgewater.de>
targos pushed a commit that referenced this pull request Sep 15, 2018
Backport-PR-URL: #22847
PR-URL: #22638
Reviewed-By: James M Snell <jasnell@gmail.com>
Reviewed-By: Ruben Bridgewater <ruben@bridgewater.de>
targos pushed a commit that referenced this pull request Sep 19, 2018
Avoids having a separate, second source of truth on this matter.

Backport-PR-URL: #22847
PR-URL: #22638
Reviewed-By: James M Snell <jasnell@gmail.com>
Reviewed-By: Ruben Bridgewater <ruben@bridgewater.de>
targos pushed a commit that referenced this pull request Sep 19, 2018
Backport-PR-URL: #22847
PR-URL: #22638
Reviewed-By: James M Snell <jasnell@gmail.com>
Reviewed-By: Ruben Bridgewater <ruben@bridgewater.de>
targos pushed a commit that referenced this pull request Sep 20, 2018
Avoids having a separate, second source of truth on this matter.

Backport-PR-URL: #22847
PR-URL: #22638
Reviewed-By: James M Snell <jasnell@gmail.com>
Reviewed-By: Ruben Bridgewater <ruben@bridgewater.de>
targos pushed a commit that referenced this pull request Sep 20, 2018
Backport-PR-URL: #22847
PR-URL: #22638
Reviewed-By: James M Snell <jasnell@gmail.com>
Reviewed-By: Ruben Bridgewater <ruben@bridgewater.de>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
author ready PRs that have at least one approval, no pending requests for changes, and a CI started. c++ Issues and PRs that require attention from people who are familiar with C++.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants