Join GitHub today
GitHub is home to over 31 million developers working together to host and review code, manage projects, and build software together.
Sign upsrc: deprecate option variables in public API #22515
Conversation
addaleax
added
C++
semver-minor
labels
Aug 24, 2018
This comment has been minimized.
This comment has been minimized.
nodejs-github-bot
added
buffer
C++
labels
Aug 24, 2018
devsnek
approved these changes
Aug 25, 2018
refack
approved these changes
Aug 25, 2018
jasnell
approved these changes
Aug 25, 2018
This comment has been minimized.
This comment has been minimized.
Which API are they exposed in? |
This comment has been minimized.
This comment has been minimized.
|
@jdalton they can be accessed by embedders/addons via |
trivikr
approved these changes
Aug 26, 2018
cjihrig
approved these changes
Aug 26, 2018
joyeecheung
approved these changes
Aug 26, 2018
tniessen
approved these changes
Aug 27, 2018
This comment has been minimized.
This comment has been minimized.
tniessen
added
the
author ready
label
Aug 27, 2018
This comment has been minimized.
This comment has been minimized.
BridgeAR
approved these changes
Aug 31, 2018
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
|
Resume CI: https://ci.nodejs.org/job/node-test-pull-request/16937/ (Side note: It does feel a bit frustrating that our policies require wasting developer time and CI resources like this. This is the third unnecessary resume CI on this PR.) |
This comment has been minimized.
This comment has been minimized.
|
Resumed in https://ci.nodejs.org/job/node-test-pull-request/16944/ ( |
This comment has been minimized.
This comment has been minimized.
|
Resume CI: https://ci.nodejs.org/job/node-test-pull-request/16945/ (edit: sorry, didn’t see @tniessen’s comment here.) |
This comment has been minimized.
This comment has been minimized.
|
Tobias’ CI is green, yay! Landed in f911e09 |
addaleax
closed this
Sep 2, 2018
addaleax
deleted the
addaleax:deprecate-option-vars
branch
Sep 2, 2018
addaleax
added a commit
that referenced
this pull request
Sep 2, 2018
targos
added a commit
that referenced
this pull request
Sep 2, 2018
This comment has been minimized.
This comment has been minimized.
|
Is there a way to avoid our own use of these variables to trigger the warning? |
addaleax commentedAug 24, 2018
These variables should never have been exposed as part of the
public API, and certainly not as variables. Using CLI options
parser is the right thing to do here, at least until we expose
some part of the options parser API publicly (which should be
possible to do now).
Checklist
make -j4 test(UNIX), orvcbuild test(Windows) passes