doc: explain why we don't use the GitHub merge button #9044

jalafel · 2016-10-12T01:42:40Z

Checklist

documentation is changed or added
commit message follows commit guidelines

Affected core subsystem(s)

Docs.

Description of change

Fixes issue: #8893.

Updates documentation on why the green GitHub merge button is not used. Also mentioned in the above issue was a question on why Reviewed-By is important to add on PRs. The documentation has also been updated to answer this.

@thealphanerd

Adds documentation and explicit reasons on why the GitHub web interface button is not used. This was explained in the referenced issue by @thealphanerd. Fixes: nodejs#8893

evanlucas · 2016-10-12T01:44:59Z

doc/onboarding.md

  * If you do, please force-push removing the merge.
+  * Reasons for not using the web interface button:
+    * The old merge method will write an ugly commit message.


I think the fact that it added a merge commit was more of a problem?

I'd avoid using the word ugly here. As @evanlucas was suggesting you might want to try something like

The old merge method would add an unnecessary merge commit

MylesBorins · 2016-10-12T02:27:24Z

@jessicaquynh thanks for submitting this! All the commit meta data looks good. With the small nit above taken care of this will be ready to land.

jalafel · 2016-10-12T03:58:19Z

@evanlucas @thealphanerd sorry about that! The file has been changed and updated.

MylesBorins

I added one last nit. If you update based on that and squash the changes into the original commit it will LGTM

MylesBorins · 2016-10-12T03:59:28Z

doc/onboarding.md

  * If you do, please force-push removing the merge.
+  * Reasons for not using the web interface button:
+    * The old merge method would add an unnecessary merge commit.


one more nit: it may make sense to not refer to these as old / new

more features are bound to come, and after a certain amount of time latest won't exactly make sense.

That makes sense! Just to clarify, do you recommend removing the qualifiers completely? So that "The old merge method ... " would simply become "The merge method ... " etc.? Thanks!

that's exactly what I was thinking

Wonderful! Changes have been updated, and commits squashed.

MylesBorins · 2016-10-12T06:55:14Z

LGTM

gibfahn · 2016-10-12T12:55:23Z

@jessicaquynh I think you still have the ugly line in the latest version:

The merge method will write an ugly commit message. -> The merge method will add an unnecessary merge commit

jalafel · 2016-10-12T16:51:55Z

@gibfahn Sorry about that error! (It is admittedly my first time using git squash) That should be fixed now. Thank you for pointing that out.

MylesBorins · 2016-10-12T17:00:14Z

looks great!

@jessicaquynh I usually use an interactive rebase for squashing and the like, let me know if you want to do a screen share some time and I can walk you through my workflow

jalafel · 2016-10-12T17:11:49Z

@thealphanerd that would be really great! I'd really appreciate that. I will message you and work out more details there :)

gibfahn · 2016-10-12T17:25:48Z

Yeah interactive rebase is amazing, I'd have no idea what I was doing without it.

lpinca · 2016-10-12T17:46:57Z

doc/onboarding.md

+    * The rebase & merge method adds metadata to the commit title.
+    * The rebase method changes the author.
+    * The squash & merge method has been known to add metadata to the commit title.
+


Silly question: is this empty line wanted? It wasn't there before.

Hi! I added the new line upon noticing that the majority of lists had two empty lines at the end before moving onto a new topic. Let me know if this should be removed!

You are right! Keep it and thanks.

lpinca

LGTM

thefourtheye · 2016-10-13T05:58:56Z

doc/onboarding.md

+    * The merge method will add an unnecessary merge commit.
+    * The rebase & merge method adds metadata to the commit title.
+    * The rebase method changes the author.
+    * The squash & merge method has been known to add metadata to the commit title.


Another point would be, if more than one author has contributed to the PR, only the latest author will be considered during the squashing.

@addaleax

Adds verbose reasons to the documentation on why the Reviewed-By metadata on a pull request is important. This was loosely mentioned as an issue in the referenced issue below, and answered by @addaleax. Ref: nodejs#8893 change wording on documentation update Changes the initial commit to the recommended, and more accurate wording. Removed time qualifiers on documentation for git merge removes the ugly wording add a new reason why autosquashing is prohibited

Qard · 2016-10-14T00:30:44Z

LGTM

MylesBorins · 2016-10-14T01:24:39Z

landed in ec7f3a1...98934d2

congrats on the first contribution @jessicaquynh 🎉

@thealphanerd

Adds documentation and explicit reasons on why the GitHub web interface button is not used. This was explained in the referenced issue by @thealphanerd. Fixes: #8893 PR-URL: #9044 Reviewed-By: Myles Borins <[email protected]> Reviewed-By: Stephen Belanger <[email protected]> Reviewed-By: Luigi Pinca <[email protected]>

@addaleax

Adds verbose reasons to the documentation on why the Reviewed-By metadata on a pull request is important. This was loosely mentioned as an issue in the referenced issue below, and answered by @addaleax. Ref: #8893 PR-URL: #9044 Reviewed-By: Myles Borins <[email protected]> Reviewed-By: Stephen Belanger <[email protected]> Reviewed-By: Luigi Pinca <[email protected]>

@thealphanerd

Adds documentation and explicit reasons on why the GitHub web interface button is not used. This was explained in the referenced issue by @thealphanerd. Fixes: #8893 PR-URL: #9044 Reviewed-By: Myles Borins <[email protected]> Reviewed-By: Stephen Belanger <[email protected]> Reviewed-By: Luigi Pinca <[email protected]>

@addaleax

Adds verbose reasons to the documentation on why the Reviewed-By metadata on a pull request is important. This was loosely mentioned as an issue in the referenced issue below, and answered by @addaleax. Ref: #8893 PR-URL: #9044 Reviewed-By: Myles Borins <[email protected]> Reviewed-By: Stephen Belanger <[email protected]> Reviewed-By: Luigi Pinca <[email protected]>

@thealphanerd

Adds documentation and explicit reasons on why the GitHub web interface button is not used. This was explained in the referenced issue by @thealphanerd. Fixes: #8893 PR-URL: #9044 Reviewed-By: Myles Borins <[email protected]> Reviewed-By: Stephen Belanger <[email protected]> Reviewed-By: Luigi Pinca <[email protected]>

@addaleax

Adds verbose reasons to the documentation on why the Reviewed-By metadata on a pull request is important. This was loosely mentioned as an issue in the referenced issue below, and answered by @addaleax. Ref: #8893 PR-URL: #9044 Reviewed-By: Myles Borins <[email protected]> Reviewed-By: Stephen Belanger <[email protected]> Reviewed-By: Luigi Pinca <[email protected]>

@thealphanerd

Adds documentation and explicit reasons on why the GitHub web interface button is not used. This was explained in the referenced issue by @thealphanerd. Fixes: #8893 PR-URL: #9044 Reviewed-By: Myles Borins <[email protected]> Reviewed-By: Stephen Belanger <[email protected]> Reviewed-By: Luigi Pinca <[email protected]>

@addaleax

Adds verbose reasons to the documentation on why the Reviewed-By metadata on a pull request is important. This was loosely mentioned as an issue in the referenced issue below, and answered by @addaleax. Ref: #8893 PR-URL: #9044 Reviewed-By: Myles Borins <[email protected]> Reviewed-By: Stephen Belanger <[email protected]> Reviewed-By: Luigi Pinca <[email protected]>

doc: explain why GitHub merge button is not used

d42d56f

Adds documentation and explicit reasons on why the GitHub web interface button is not used. This was explained in the referenced issue by @thealphanerd. Fixes: nodejs#8893

nodejs-github-bot added the doc Issues and PRs related to the documentations. label Oct 12, 2016

jalafel mentioned this pull request Oct 12, 2016

doc: explain why we don't use the GitHub merge button #8893

Closed

evanlucas reviewed Oct 12, 2016

View reviewed changes

MylesBorins suggested changes Oct 12, 2016

View reviewed changes

jalafel force-pushed the issue#8893 branch from ef09a06 to b35ffc7 Compare October 12, 2016 05:48

MylesBorins approved these changes Oct 12, 2016

View reviewed changes

jalafel force-pushed the issue#8893 branch from b35ffc7 to e876856 Compare October 12, 2016 16:49

lpinca reviewed Oct 12, 2016

View reviewed changes

lpinca approved these changes Oct 12, 2016

View reviewed changes

thefourtheye reviewed Oct 13, 2016

View reviewed changes

jalafel force-pushed the issue#8893 branch from e876856 to 177d880 Compare October 13, 2016 20:49

MylesBorins closed this Oct 14, 2016

MylesBorins added land-on-v4.x labels Nov 11, 2016

This was referenced Nov 22, 2016

v6.9.2 proposal #9735

Merged

v4.7.0 proposal #9736

Merged

aliscco mentioned this pull request Dec 1, 2023

[Snyk] Fix for 2 vulnerabilities aliscco/alisco-node#903

Open

leonardoadame mentioned this pull request Dec 3, 2023

[Snyk] Fix for 1 vulnerabilities leonardoadame/node#508

Open

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

doc: explain why we don't use the GitHub merge button #9044

doc: explain why we don't use the GitHub merge button #9044

jalafel commented Oct 12, 2016

evanlucas Oct 12, 2016

MylesBorins Oct 12, 2016 •

edited

Loading

MylesBorins commented Oct 12, 2016

jalafel commented Oct 12, 2016

MylesBorins left a comment

MylesBorins Oct 12, 2016

jalafel Oct 12, 2016

MylesBorins Oct 12, 2016

jalafel Oct 12, 2016

MylesBorins commented Oct 12, 2016

gibfahn commented Oct 12, 2016

jalafel commented Oct 12, 2016

MylesBorins commented Oct 12, 2016

jalafel commented Oct 12, 2016

gibfahn commented Oct 12, 2016

lpinca Oct 12, 2016

jalafel Oct 12, 2016

lpinca Oct 13, 2016

lpinca left a comment

thefourtheye Oct 13, 2016

Qard commented Oct 14, 2016

MylesBorins commented Oct 14, 2016

doc: explain why we don't use the GitHub merge button #9044

doc: explain why we don't use the GitHub merge button #9044

Conversation

jalafel commented Oct 12, 2016

Checklist

Affected core subsystem(s)

Description of change

Choose a reason for hiding this comment

MylesBorins Oct 12, 2016 • edited Loading

Choose a reason for hiding this comment

MylesBorins commented Oct 12, 2016

jalafel commented Oct 12, 2016

MylesBorins left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

MylesBorins commented Oct 12, 2016

gibfahn commented Oct 12, 2016

jalafel commented Oct 12, 2016

MylesBorins commented Oct 12, 2016

jalafel commented Oct 12, 2016

gibfahn commented Oct 12, 2016

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

lpinca left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Qard commented Oct 14, 2016

MylesBorins commented Oct 14, 2016

MylesBorins Oct 12, 2016 •

edited

Loading