Skip to content
This repository was archived by the owner on Oct 15, 2020. It is now read-only.

deps,src,test: fixing lint errors#465

Closed
MSLaguana wants to merge 1 commit intonodejs:masterfrom
MSLaguana:fixLintErrors
Closed

deps,src,test: fixing lint errors#465
MSLaguana wants to merge 1 commit intonodejs:masterfrom
MSLaguana:fixLintErrors

Conversation

@MSLaguana
Copy link
Copy Markdown
Contributor

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

deps, src, test

@kfarnung
Copy link
Copy Markdown
Contributor

Nice! I didn't see this one before creating my PR, I'll rebase my changes on this afterwards.

Comment thread src/node_api_jsrt.cc Outdated
reinterpret_cast<node::async_context*>(async_context_handle);

// TODO: It'd be nice if we could remove the dependency on v8::*
// TODO(jithomso): It'd be nice if we could remove the dependency on v8::*
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

I'd use your GitHub ID

Comment thread src/node_api_jsrt.cc Outdated
new node::InternalCallbackScope(node::Environment::GetCurrent(env->isolate),
resource,
*node_async_context));
new node::InternalCallbackScope(
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Generally we use two indents for continuations from the previous line.

Comment thread test/cctest/node_test_fixture.h Outdated
#if ENABLE_TTD_NODE
context_ = node::NewContext(isolate, false); // TODO: should we support TTD in cctest?
context_ = node::NewContext(isolate, false);
// TODO(jithomso): should we support TTD in cctest?
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

nit: I'd move the comment above the line, also use your GitHub ID

Copy link
Copy Markdown
Contributor

@kfarnung kfarnung left a comment

Choose a reason for hiding this comment

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

LGTM with minor fixes

@kfarnung kfarnung mentioned this pull request Feb 14, 2018
2 tasks
@MSLaguana
Copy link
Copy Markdown
Contributor Author

Done. Waiting on the node pump to complete before I finish this PR.

MSLaguana added a commit that referenced this pull request Feb 14, 2018
PR-URL: #465
Reviewed-By: Kyle Farnung <[email protected]>
Reviewed-By: Seth Brenith <[email protected]>
@MSLaguana
Copy link
Copy Markdown
Contributor Author

Landed in c057e77

@MSLaguana MSLaguana closed this Feb 14, 2018
kfarnung pushed a commit to kfarnung/node-chakracore that referenced this pull request Feb 23, 2018
PR-URL: nodejs#465
Reviewed-By: Kyle Farnung <[email protected]>
Reviewed-By: Seth Brenith <[email protected]>
kfarnung pushed a commit to kfarnung/node-chakracore that referenced this pull request Mar 6, 2018
PR-URL: nodejs#465
Reviewed-By: Kyle Farnung <[email protected]>
Reviewed-By: Seth Brenith <[email protected]>
kfarnung pushed a commit to kfarnung/node-chakracore that referenced this pull request Mar 7, 2018
PR-URL: nodejs#465
Reviewed-By: Kyle Farnung <[email protected]>
Reviewed-By: Seth Brenith <[email protected]>
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants