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

src: elevate v8 namespaces of node_trace_events.cc #24469

Closed
wants to merge 7 commits into from

Conversation

@Jayasankar-m
Copy link
Contributor

commented Nov 19, 2018

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
@Jayasankar-m

This comment has been minimized.

Copy link
Contributor Author

commented Nov 19, 2018

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

@gireeshpunathil : Please have a review.

src/node_trace_events.cc Outdated Show resolved Hide resolved
@gireeshpunathil

This comment has been minimized.

Copy link
Member

commented Nov 19, 2018

@Jayasankar-m - thanks, now that this is your second (or third?) PR, shall I request you to address @cjihrig 's comment to be addressed (remove unwanted changes) and squash all commits into one? don't worry about the mistakes, it only strengthens your learning, to the spirit of the code & learn, thanks!

@Jayasankar-m

This comment has been minimized.

Copy link
Contributor Author

commented Nov 19, 2018

@jasnell
Copy link
Member

left a comment

LGTM with the unrelated white space change removed.

@Jayasankar-m

This comment has been minimized.

Copy link
Contributor Author

commented Nov 19, 2018

@Jayasankar-m - thanks, now that this is your second (or third?) PR, shall I request you to address @cjihrig 's comment to be addressed (remove unwanted changes) and squash all commits into one? don't worry about the mistakes, it only strengthens your learning, to the spirit of the code & learn, thanks!

@gireeshpunathil - Have removed the unwanted changes... Regarding squashing it seems my forked copy had some other changes..need to figure out..

@gireeshpunathil

This comment has been minimized.

Copy link
Member

commented Nov 19, 2018

@Jayasankar-m - thanks, looks great now! leave the squashing part then, one of us will do it while landing.

@Jayasankar-m

This comment has been minimized.

Copy link
Contributor Author

commented Nov 19, 2018

@gireeshpunathil

This comment has been minimized.

@gireeshpunathil

This comment has been minimized.

Copy link
Member

commented Nov 20, 2018

landed as 15c2491 , thanks!

gireeshpunathil added a commit that referenced this pull request Nov 20, 2018
src: elevate v8 namespaces of node_trace_events.cc
PR-URL: #24469
Reviewed-By: James M Snell <jasnell@gmail.com>
Reviewed-By: Sakthipriyan Vairamani <thechargingvolcano@gmail.com>
Reviewed-By: Colin Ihrig <cjihrig@gmail.com>
Reviewed-By: Gireesh Punathil <gpunathi@in.ibm.com>
targos added a commit that referenced this pull request Nov 20, 2018
src: elevate v8 namespaces of node_trace_events.cc
PR-URL: #24469
Reviewed-By: James M Snell <jasnell@gmail.com>
Reviewed-By: Sakthipriyan Vairamani <thechargingvolcano@gmail.com>
Reviewed-By: Colin Ihrig <cjihrig@gmail.com>
Reviewed-By: Gireesh Punathil <gpunathi@in.ibm.com>
rvagg added a commit that referenced this pull request Nov 28, 2018
src: elevate v8 namespaces of node_trace_events.cc
PR-URL: #24469
Reviewed-By: James M Snell <jasnell@gmail.com>
Reviewed-By: Sakthipriyan Vairamani <thechargingvolcano@gmail.com>
Reviewed-By: Colin Ihrig <cjihrig@gmail.com>
Reviewed-By: Gireesh Punathil <gpunathi@in.ibm.com>
@BridgeAR BridgeAR referenced this pull request Dec 5, 2018
4 of 4 tasks complete
refack added a commit to refack/node that referenced this pull request Jan 14, 2019
src: elevate v8 namespaces of node_trace_events.cc
PR-URL: nodejs#24469
Reviewed-By: James M Snell <jasnell@gmail.com>
Reviewed-By: Sakthipriyan Vairamani <thechargingvolcano@gmail.com>
Reviewed-By: Colin Ihrig <cjihrig@gmail.com>
Reviewed-By: Gireesh Punathil <gpunathi@in.ibm.com>
BethGriggs added a commit that referenced this pull request Feb 11, 2019
src: elevate v8 namespaces of node_trace_events.cc
PR-URL: #24469
Reviewed-By: James M Snell <jasnell@gmail.com>
Reviewed-By: Sakthipriyan Vairamani <thechargingvolcano@gmail.com>
Reviewed-By: Colin Ihrig <cjihrig@gmail.com>
Reviewed-By: Gireesh Punathil <gpunathi@in.ibm.com>
@BethGriggs BethGriggs referenced this pull request Feb 12, 2019
rvagg added a commit that referenced this pull request Feb 28, 2019
src: elevate v8 namespaces of node_trace_events.cc
PR-URL: #24469
Reviewed-By: James M Snell <jasnell@gmail.com>
Reviewed-By: Sakthipriyan Vairamani <thechargingvolcano@gmail.com>
Reviewed-By: Colin Ihrig <cjihrig@gmail.com>
Reviewed-By: Gireesh Punathil <gpunathi@in.ibm.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
7 participants
You can’t perform that action at this time.