FazBrowse GitHub Viewer | Trending |
URL:
| Home
Tools: [Download Repo ZIP]   [Original HTTPS Page]

src: add READONLY_STRING_PROPERTY and simplify config by maclover7 · Pull Request #22222 · nodejs/node · GitHub

/ node Public

src: add READONLY_STRING_PROPERTY and simplify config - #22222

Closed
maclover7 wants to merge 2 commits into
nodejs:masterfrom
maclover7:jm-node-config
ConversationCommits (2)ChecksFiles changed
Closed

src: add READONLY_STRING_PROPERTY and simplify config#22222
maclover7 wants to merge 2 commits into
nodejs:masterfrom
maclover7:jm-node-config

Conversation

maclover7 commented Aug 9, 2018 •
edited
Loading

Copy link
Copy Markdown
Contributor

Bit of tidying up where we set different config values.

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

src: add READONLY_STRING_PROPERTY and simplify config
a645f82 
Bit of tidying up where we set different config values..

nodejs-github-bot commented Aug 9, 2018

Copy link
Copy Markdown
Collaborator

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

nodejs-github-bot added the c++ Issues and PRs that require attention from people who are familiar with C++. label Aug 9, 2018
addaleax approved these changes Aug 9, 2018
View reviewed changes

addaleax 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.

Choose a reason Spam Abuse Off Topic Outdated Duplicate Resolved Low Quality

LGTM but can we fast track this? I like it, but it’s, like, very directly conflicting with what I’m working on at this point. 😄

Comment thread src/node_config.cc Outdated

#define READONLY_STRING_PROPERTY(obj, str, val) \
do { \
obj->DefineOwnProperty(context, \

addaleax Aug 9, 2018

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.

Choose a reason Spam Abuse Off Topic Outdated Duplicate Resolved Low Quality

It’s a best practice to parenthesize macro parameters, i.e. use (obj)->DefineOwnProperty(…), even if this is probably not an issue for now :)

Comment thread src/node_config.cc Outdated
String::NewFromUtf8( \
isolate, \
val.data(), \
v8::NewStringType::kNormal).ToLocalChecked(), \

addaleax Aug 9, 2018

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.

Choose a reason Spam Abuse Off Topic Outdated Duplicate Resolved Low Quality

tiny nit: 4 spaces for statement continuations

maclover7 commented Aug 9, 2018

Copy link
Copy Markdown
Contributor Author

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

maclover7 commented Aug 11, 2018

Copy link
Copy Markdown
Contributor Author

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

maclover7 added the author ready PRs that have at least one approval, no outstanding review comments, and a CI started. label Aug 11, 2018

maclover7 commented Aug 12, 2018

Copy link
Copy Markdown
Contributor Author

Landed in 30b5b84

maclover7 closed this Aug 12, 2018
maclover7 deleted the jm-node-config branch August 12, 2018 00:28
maclover7 added a commit that referenced this pull request Aug 12, 2018
src: add READONLY_STRING_PROPERTY and simplify config
30b5b84 
Bit of tidying up where we set different config values.

PR-URL: #22222
Reviewed-By: Anna Henningsen <anna@addaleax.net>
Reviewed-By: Richard Lau <riclau@uk.ibm.com>
Reviewed-By: Colin Ihrig <cjihrig@gmail.com>
Reviewed-By: Gus Caplan <me@gus.host>
Reviewed-By: James M Snell <jasnell@gmail.com>
targos pushed a commit that referenced this pull request Aug 12, 2018
src: add READONLY_STRING_PROPERTY and simplify config
b3eb10d 
Bit of tidying up where we set different config values.

PR-URL: #22222
Reviewed-By: Anna Henningsen <anna@addaleax.net>
Reviewed-By: Richard Lau <riclau@uk.ibm.com>
Reviewed-By: Colin Ihrig <cjihrig@gmail.com>
Reviewed-By: Gus Caplan <me@gus.host>
Reviewed-By: James M Snell <jasnell@gmail.com>
rvagg mentioned this pull request Aug 13, 2018
Merged
This was referenced Aug 16, 2018
Closed
Closed
Closed
Closed
Closed
Closed
Closed
Closed
Closed
Closed
firass111 pushed a commit to firass111/Project_node1 that referenced this pull request Apr 16, 2025
src: add READONLY_STRING_PROPERTY and simplify config
2737b46 
Bit of tidying up where we set different config values.

PR-URL: nodejs/node#22222
Reviewed-By: Anna Henningsen <anna@addaleax.net>
Reviewed-By: Richard Lau <riclau@uk.ibm.com>
Reviewed-By: Colin Ihrig <cjihrig@gmail.com>
Reviewed-By: Gus Caplan <me@gus.host>
Reviewed-By: James M Snell <jasnell@gmail.com>
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters. Learn more about bidirectional Unicode characters
Show hidden characters
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

jasnell jasnell approved these changes
addaleax addaleax approved these changes
cjihrig cjihrig approved these changes
richardlau richardlau approved these changes
+1 more reviewer
devsnek devsnek approved these changes
Reviewers whose approvals may not affect merge requirements

Assignees

No one assigned

Labels

author ready PRs that have at least one approval, no outstanding review comments, and a CI started. c++ Issues and PRs that require attention from people who are familiar with C++.

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

7 participants

Back | FazBrowse Home | New Git URL