Skip to content

report: refactor configuration management - #26414

Merged
cjihrig merged 2 commits into
nodejs:masterfrom
cjihrig:sync
Mar 5, 2019
Merged

report: refactor configuration management#26414
cjihrig merged 2 commits into
nodejs:masterfrom
cjihrig:sync

Conversation

@cjihrig

@cjihrigcjihrig commented Mar 3, 2019

Copy link
Copy Markdown
Contributor

This commit removes process.report.setOptions(). Instead of using complex configuration synchronization between C++ and JS, this commit introduces individual getters and setters.

Checklist
  • make -j4 test (UNIX), or vcbuild test (Windows) passes
  • tests and/or benchmarks are included
  • documentation is changed or added
  • commit message follows commit guidelines

@nodejs-github-bot

nodejs-github-bot commented Mar 3, 2019

Copy link
Copy Markdown
Collaborator

@cjihrig build started: https://ci.nodejs.org/blue/organizations/jenkins/node-test-pull-request-lite-pipeline/detail/node-test-pull-request-lite-pipeline/2750/pipeline

@nodejs-github-botnodejs-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. labels Mar 3, 2019
richardlau
richardlau reviewed Mar 3, 2019
View reviewed changes
Comment threaddoc/api/process.md Outdated
Comment threadtest/report/test-report-config.js Outdated
Comment threadtest/report/test-report-config.js Outdated
@richardlaurichardlau added the report Issues and PRs related to process.report. label Mar 3, 2019
@cjihrig

cjihrig commented Mar 5, 2019

Copy link
Copy Markdown
ContributorAuthor

@richardlau@jasnell nits addressed

jasnell
jasnell reviewed Mar 5, 2019
View reviewed changes
Comment threadlib/internal/process/report.js Outdated
richardlau
richardlau approved these changes Mar 5, 2019
View reviewed changes

@richardlaurichardlau left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

LGTM with one suggestion and once the test is fixed for Windows.

Comment threadlib/internal/process/report.js Outdated
Comment threadtest/report/test-report-config.js Outdated
@cjihrig

cjihrig commented Mar 5, 2019

Copy link
Copy Markdown
ContributorAuthor

@richardlau@jasnell nits addressed (again)

@richardlau

richardlau commented Mar 5, 2019

Copy link
Copy Markdown
Member

There are some C++ linter failures:

https://travis-ci.com/nodejs/node/jobs/182405108#L457-L462

Running C++ linter...
File "src/node_report_module.cc" does not use "FIXED_ONE_BYTE_STRING"
File "src/node_report_module.cc" does not use "PerIsolateOptions"
File "src/node_report_module.cc" does not use "Array"
File "src/node_report_module.cc" does not use "V8"
make[1]: *** [tools/.cpplintstamp] Error 1

@cjihrig

cjihrig commented Mar 5, 2019

Copy link
Copy Markdown
ContributorAuthor

OK, Travis seems to be happy now.

geek
geek approved these changes Mar 5, 2019
View reviewed changes
@cjihrig
report: refactor configuration management
30ee277
This commit removes process.report.setOptions(). Instead of
using complex configuration synchronization between C++ and
JS, this commit introduces individual getters and setters.
PR-URL: nodejs#26414
Reviewed-By: Richard Lau <riclau@uk.ibm.com>
Reviewed-By: Wyatt Preul <wpreul@gmail.com>
@cjihrig
report: warn on process.report object access
cc4e8e0
Reduce the number of emitExperimentalWarning() call sites by
making process.report emit a warning on access instead of each
individual report function.
PR-URL: nodejs#26414
Reviewed-By: Richard Lau <riclau@uk.ibm.com>
Reviewed-By: Wyatt Preul <wpreul@gmail.com>
@cjihrig

cjihrig commented Mar 5, 2019

Copy link
Copy Markdown
ContributorAuthor

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

@cjihrig

cjihrig commented Mar 5, 2019

Copy link
Copy Markdown
ContributorAuthor

Landed in c78788a...cc4e8e0. Thanks for the reviews!

@cjihrigcjihrig closed this Mar 5, 2019
@cjihrig
cjihrig deleted the sync branch March 5, 2019 20:37
@cjihrig
cjihrig merged commit cc4e8e0 into nodejs:masterMar 5, 2019
BridgeAR pushed a commit to BridgeAR/node that referenced this pull request Mar 12, 2019
@cjihrig@BridgeAR
report: refactor configuration management
9f446a1
This commit removes process.report.setOptions(). Instead of
using complex configuration synchronization between C++ and
JS, this commit introduces individual getters and setters.
PR-URL: nodejs#26414
Reviewed-By: Richard Lau <riclau@uk.ibm.com>
Reviewed-By: Wyatt Preul <wpreul@gmail.com>
BridgeAR pushed a commit to BridgeAR/node that referenced this pull request Mar 12, 2019
@cjihrig@BridgeAR
report: warn on process.report object access
2be9e80
Reduce the number of emitExperimentalWarning() call sites by
making process.report emit a warning on access instead of each
individual report function.
PR-URL: nodejs#26414
Reviewed-By: Richard Lau <riclau@uk.ibm.com>
Reviewed-By: Wyatt Preul <wpreul@gmail.com>
@BridgeARBridgeAR mentioned this pull request Mar 13, 2019
Merged
Sign up for freeto join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@jasnelljasnelljasnell left review comments
@richardlaurichardlaurichardlau approved these changes
+1 more reviewer
@geekgeekgeek approved these changes
Reviewers whose approvals may not affect merge requirements

Assignees

No one assigned

Labels

c++Issues and PRs that require attention from people who are familiar with C++.lib / srcIssues and PRs related to general changes in the lib or src directory.reportIssues and PRs related to process.report.

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

5 participants

@cjihrig@nodejs-github-bot@richardlau@jasnell@geek