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 repeated artifacts #24427

Merged
merged 1 commit into from Nov 18, 2018

Conversation

@Shubhamurkade
Copy link
Contributor

commented Nov 17, 2018

There are several places where v8 artifacts appear with scope resolution operator inline with the source. Elevated the places where the same artifacts were used repeatedly. The places where an artifact was used only once has been retained as it is.

@targos
targos approved these changes Nov 17, 2018

@targos targos added the fast-track label Nov 17, 2018

@targos

This comment has been minimized.

Copy link
Member

commented Nov 17, 2018

👍 to fast-track

@targos

This comment has been minimized.

Copy link
Member

commented Nov 17, 2018

The first line of the commit message should be shorter (<=50 characters if possible)

@Shubhamurkade Shubhamurkade changed the title src: Elevate v8 namespaces of repeatedly referenced artifacts with 'using' keyword in src/inspector_agent.cc src: elevate v8 namespaces of repeated artifacts Nov 17, 2018

@Shubhamurkade

This comment has been minimized.

Copy link
Contributor Author

commented Nov 17, 2018

The first line of the commit message should be shorter (<=50 characters if possible)

Does it look good now?

@refack
refack approved these changes Nov 17, 2018
@refack

This comment has been minimized.

@refack

This comment has been minimized.

Copy link
Member

commented Nov 17, 2018

Hello @Shubhamurkade and welcome, and thank you for your contribution 🥇
If you are not familiar with our review and landing process, it's covered in CONTRIBUTING.md

P.S. If you have any questions you can also feel free to contact me directly.

@refack

This comment has been minimized.

Copy link
Member

commented Nov 17, 2018

FYI: @Shubhamurkade if you'd like you can associate your email with your GitHub account. That way we and GitHub will credit the code contributions you make to you. See https://github.com/settings/emails and the guide at https://help.github.com/articles/setting-your-commit-email-address-on-github/

@refack refack added the blocked label Nov 17, 2018

@Shubhamurkade

This comment has been minimized.

Copy link
Contributor Author

commented Nov 18, 2018

FYI: @Shubhamurkade if you'd like you can associate your email with your GitHub account. That way we and GitHub will credit the code contributions you make to you. See https://github.com/settings/emails and the guide at https://help.github.com/articles/setting-your-commit-email-address-on-github/

Hi @refack! I already have an email address associated with my github account. Please look at the attached image.
email

@refack

This comment has been minimized.

Copy link
Member

commented Nov 18, 2018

@Shubhamurkade it seems like that's not the email you have setup in your local git. You can see the GitHub doesn't recognize it in this screen 787db0f
image.
and in the header of git commit metadata - https://patch-diff.githubusercontent.com/raw/nodejs/node/pull/24427.patch

I'll try to fix it for this PR while landing it. ✔️

@refack refack removed the blocked label Nov 18, 2018

@refack refack force-pushed the Shubhamurkade:namespace_change branch from 787db0f to df8d89f Nov 18, 2018

refack added a commit to Shubhamurkade/node that referenced this pull request Nov 18, 2018
src: elevate repeated use of v8 namespaced type
PR-URL: nodejs#24427
Reviewed-By: Michaël Zasso <targos@protonmail.com>
Reviewed-By: Refael Ackermann <refack@gmail.com>
Reviewed-By: Anna Henningsen <anna@addaleax.net>
Reviewed-By: Colin Ihrig <cjihrig@gmail.com>
Reviewed-By: Gireesh Punathil <gpunathi@in.ibm.com>
src: elevate repeated use of v8 namespaced type
PR-URL: #24427
Reviewed-By: Michaël Zasso <targos@protonmail.com>
Reviewed-By: Refael Ackermann <refack@gmail.com>
Reviewed-By: Anna Henningsen <anna@addaleax.net>
Reviewed-By: Colin Ihrig <cjihrig@gmail.com>
Reviewed-By: Gireesh Punathil <gpunathi@in.ibm.com>

@refack refack force-pushed the Shubhamurkade:namespace_change branch from df8d89f to ea65519 Nov 18, 2018

@refack refack merged commit ea65519 into nodejs:master Nov 18, 2018

1 of 2 checks passed

Travis CI - Pull Request Build Errored
Details
Travis CI - Branch Build Passed
Details
@refack

This comment has been minimized.

Copy link
Member

commented Nov 18, 2018

@Shubhamurkade congratulations 🎉 on getting promoted by GitHub form:
image
to
image

Hope to see you contributing again in the future!

@Trott

This comment has been minimized.

Copy link
Member

commented Nov 18, 2018

Thanks for the contribution! 🎉

(If you're interested in other possible contributions to Node.js but don't have a good idea of where to start looking, some ideas are posted at https://www.nodetodo.org/next-steps/.)

@Shubhamurkade

This comment has been minimized.

Copy link
Contributor Author

commented Nov 19, 2018

Thanks for the contribution! 🎉

(If you're interested in other possible contributions to Node.js but don't have a good idea of where to start looking, some ideas are posted at https://www.nodetodo.org/next-steps/.)

Thank you!

targos added a commit that referenced this pull request Nov 19, 2018
src: elevate repeated use of v8 namespaced type
PR-URL: #24427
Reviewed-By: Michaël Zasso <targos@protonmail.com>
Reviewed-By: Refael Ackermann <refack@gmail.com>
Reviewed-By: Anna Henningsen <anna@addaleax.net>
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 repeated use of v8 namespaced type
PR-URL: #24427
Reviewed-By: Michaël Zasso <targos@protonmail.com>
Reviewed-By: Refael Ackermann <refack@gmail.com>
Reviewed-By: Anna Henningsen <anna@addaleax.net>
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
codebytere added a commit that referenced this pull request Jan 13, 2019
src: elevate repeated use of v8 namespaced type
PR-URL: #24427
Reviewed-By: Michaël Zasso <targos@protonmail.com>
Reviewed-By: Refael Ackermann <refack@gmail.com>
Reviewed-By: Anna Henningsen <anna@addaleax.net>
Reviewed-By: Colin Ihrig <cjihrig@gmail.com>
Reviewed-By: Gireesh Punathil <gpunathi@in.ibm.com>
refack added a commit to refack/node that referenced this pull request Jan 14, 2019
src: elevate repeated use of v8 namespaced type
PR-URL: nodejs#24427
Reviewed-By: Michaël Zasso <targos@protonmail.com>
Reviewed-By: Refael Ackermann <refack@gmail.com>
Reviewed-By: Anna Henningsen <anna@addaleax.net>
Reviewed-By: Colin Ihrig <cjihrig@gmail.com>
Reviewed-By: Gireesh Punathil <gpunathi@in.ibm.com>
@codebytere codebytere referenced this pull request Jan 15, 2019
MylesBorins added a commit that referenced this pull request Jan 29, 2019
src: elevate repeated use of v8 namespaced type
PR-URL: #24427
Reviewed-By: Michaël Zasso <targos@protonmail.com>
Reviewed-By: Refael Ackermann <refack@gmail.com>
Reviewed-By: Anna Henningsen <anna@addaleax.net>
Reviewed-By: Colin Ihrig <cjihrig@gmail.com>
Reviewed-By: Gireesh Punathil <gpunathi@in.ibm.com>
codebytere added a commit that referenced this pull request Jan 29, 2019
src: elevate repeated use of v8 namespaced type
PR-URL: #24427
Reviewed-By: Michaël Zasso <targos@protonmail.com>
Reviewed-By: Refael Ackermann <refack@gmail.com>
Reviewed-By: Anna Henningsen <anna@addaleax.net>
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
8 participants
You can’t perform that action at this time.