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

module: named anonymous function for debugging purposes #20811

Closed
wants to merge 1 commit into from

Conversation

ndangles
Copy link
Contributor

@ndangles ndangles commented May 18, 2018

My first contribution

This commit is to help in the effort to name all anonymous
functions to help when heap debugging. The issue asked for
any functions to be updated that are not on a prototype
and this file contains one function that meets those needs.

A previous pull request (13849) was not completed and did
not meet the requirements of the issue so this PR looks
to complete that.

Refs: #8913

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

@tniessen
Copy link
Member

Thank you for your contribution, Nicholas! 😃 Please note that the commit metadata does not match your GitHub account, so unless you either change your Git configuration or add the commit email address to your GitHub account, the commit will not be associated with your account when landed. Additionally, please double-check whether your PR actually Fixes: #8913. Fixes means that the issue as a whole will be closed when landing this PR, which might not be desired if you are only addressing parts of the issue. Use Refs for partial fixes.

@ndangles
Copy link
Contributor Author

@tniessen Hello! I think I resolved the email issue.

The following commands were ran:
git rebase -i bb7912f
git commit --amend --author="ndangles [email protected]"
git rebase --continue
git push origin my-branch

@benjamingr
Copy link
Member

Whoever is landing it on our end could just land just this one bb7912f and not the accidental commits from the pull/merge I think.

Is th email you're using associated with your GitHub account? https://help.github.com/articles/why-are-my-contributions-not-showing-up-on-my-profile/

@benjamingr
Copy link
Member

Also, thanks for making a contribution, we genuinely appreciate it :]

@ndangles
Copy link
Contributor Author

@benjamingr Yes, the email is associated with my Github account. Also, How I can I prevent accidental commits in the future? My intent was just to update the email metadata and not commit additional files.

Thank you!

@benjamingr
Copy link
Member

@ndangles well, generally never add merge commits and if there is a conflict resolve it by rebasing the new work on top of what was already merged.

We have a pretty detailed guide at Pull Requests - you can also check out how we end up landing your code.

Honestly at the first few commits can be challenging. We don't blame people for not getting it 100% right and we just appreciate the effort and contribution.

It's pretty tricky to do initially and we understand that - don't worry about it :)

@ndangles
Copy link
Contributor Author

ndangles commented May 18, 2018

@benjamingr This is my first time contributing to such a huge project so definitely a lot different then working on my own personal projects but I am willing to learn and hope to continue to contribute and I appreciate your help and patience

@tniessen
Copy link
Member

@ndangles It seems like you added some more commits instead of updating the metadata of your own commit. We can fix that later, but feel free to follow these steps to update your commit with the correct metadata:

# Let's first remove the unwanted commits from your branch:
git checkout issue-8913
git reset --hard bb7912f577d667be2343070869fcece1f437cc13

# If you did not add the upstream remote yet, do so now:
git remote add upstream https://github.com/nodejs/node.git

# Rebasing is optional, but why not?
git fetch upstream master
git rebase upstream/master

# Your commit should still be the most recent commit at this point, but let's check:
git log --oneline -1

# Let's fix the committer metadata:
# (Note that most people use their real name here instead of their GitHub handle, but feel
# free to use whatever you want.)
git config user.email "[email protected]"
git config user.name "Nicholas Dangles"
# Finally, we also need to fix the author metadata:
git commit --amend --author="Nicholas Dangles <[email protected]>"

# Check that the metadata is correct:
git show --format=fuller --quiet

# Update your PR:
git push origin issue-8913 --force-with-lease

If everything worked, this PR should then list a single commit which is associated with your account.

My first contribution

This commit is to help in the effort to name all anonymous
functions to help when heap debugging. The issue asked for
any functions to be updated that are not on a prototype
and this file contains one function that meets those needs.

A previous pull request (13849) was not completed and did
not meet the requirements of the issue so this PR looks
to complete that.

Refs: nodejs#8913
@ndangles
Copy link
Contributor Author

@tniessen Okay awesome, looked like everything worked. Thank you very much, that was very informative and I understand more now.

Copy link
Member

@tniessen tniessen left a comment

Choose a reason for hiding this comment

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

Commit message should be fixed while landing.

@ndangles
Copy link
Contributor Author

@tniessen Are there any other things I need to do from my end?

@benjamingr
Copy link
Member

@ndangles pull requests in Node.js stay open for 48-72 hours (72 in this case because this is a weekend) before they are typically merged in order to give collaborators a chance to comment on them.*

If all goes well in a few days this will be merged as your first (and hopefully not last :)) commit in Node.js.

* we do sometimes fast-track things but in this case I don't think that's a big deal

@jasnell
Copy link
Member

jasnell commented May 23, 2018

@benjamingr benjamingr added the author ready PRs that have at least one approval, no pending requests for changes, and a CI started. label May 23, 2018
Copy link
Member

@benjamingr benjamingr left a comment

Choose a reason for hiding this comment

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

I could have sworn I approved it already.

@tniessen
Copy link
Member

Landed in 65dbc52.

@tniessen tniessen closed this May 25, 2018
tniessen pushed a commit that referenced this pull request May 25, 2018
This commit is to help in the effort to name all anonymous
functions to help when heap debugging. The issue asked for
any functions to be updated that are not on a prototype
and this file contains one function that meets those needs.

A previous pull request (13849) was not completed and did
not meet the requirements of the issue so this PR looks
to complete that.

PR-URL: #20811
Refs: #8913
Reviewed-By: Tobias Nießen <[email protected]>
Reviewed-By: James M Snell <[email protected]>
Reviewed-By: Trivikram Kamat <[email protected]>
Reviewed-By: Benjamin Gruenbaum <[email protected]>
Reviewed-By: Ruben Bridgewater <[email protected]>
@ndangles
Copy link
Contributor Author

@tniessen @benjamingr I appreciate the guidance and help on my first contribution. Thank you!

targos pushed a commit that referenced this pull request May 25, 2018
This commit is to help in the effort to name all anonymous
functions to help when heap debugging. The issue asked for
any functions to be updated that are not on a prototype
and this file contains one function that meets those needs.

A previous pull request (13849) was not completed and did
not meet the requirements of the issue so this PR looks
to complete that.

PR-URL: #20811
Refs: #8913
Reviewed-By: Tobias Nießen <[email protected]>
Reviewed-By: James M Snell <[email protected]>
Reviewed-By: Trivikram Kamat <[email protected]>
Reviewed-By: Benjamin Gruenbaum <[email protected]>
Reviewed-By: Ruben Bridgewater <[email protected]>
@MylesBorins MylesBorins mentioned this pull request May 29, 2018
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
author ready PRs that have at least one approval, no pending requests for changes, and a CI started.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

6 participants