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.

Sign up for GitHub

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

Jump to bottom

doc: add note regarding commit message trailers #56736

Merged
merged 1 commit into from
Jan 26, 2025
Merged

doc: add note regarding commit message trailers #56736

merged 1 commit into from
Jan 26, 2025
+5 −0

Conversation

dario-piotrowicz
Copy link
Contributor

@dario-piotrowicz dario-piotrowicz commented Jan 24, 2025 •
edited
Loading

@nodejs-github-bot
Copy link
Collaborator

Review requested:

  • @nodejs/tsc

@nodejs-github-bot nodejs-github-bot added the doc Issues and PRs related to the documentations. label Jan 24, 2025
anonrig
anonrig reviewed Jan 24, 2025
View reviewed changes
doc/contributing/pull-requests.md Outdated Show resolved Hide resolved
doc/contributing/pull-requests.md Outdated Show resolved Hide resolved
doc/contributing/pull-requests.md Outdated Show resolved Hide resolved
@dario-piotrowicz dario-piotrowicz force-pushed the dario/contributing-clarify-fixes-optional branch 2 times, most recently from 91efab6 to 607e853 Compare January 24, 2025 09:55
@aduh95
Copy link
Contributor

aduh95 commented Jan 24, 2025

nit: Im not a fan of the commit message. I'm not sure the added note clarifies the trailers are optional – and I don't know if we want them to be optional, it's useful to have them, in fact that's why the bot does add them when landing PRs. Also I'm not sure what PR commit means. If someone reads the commit title in the CHANGELOG, they would likely get confused and make wrong assumption of this change actually is. I can suggest doc: add note regarding commit message trailers as an alternative, but feel free to chose a different one or to ignore.

@dario-piotrowicz @anonrig @aduh95
doc: add note regarding commit message trailers
b8be869 
Co-authored-by: Yagiz Nizipli <yagiz@nizipli.com>
Co-authored-by: Antoine du Hamel <duhamelantoine1995@gmail.com>
@dario-piotrowicz dario-piotrowicz force-pushed the dario/contributing-clarify-fixes-optional branch from 607e853 to b8be869 Compare January 24, 2025 10:47
@dario-piotrowicz
Copy link
Contributor Author

nit: Im not a fan of the commit message. I'm not sure the added note clarifies the trailers are optional – and I don't know if we want them to be optional, it's useful to have them, in fact that's why the bot does add them when landing PRs. Also I'm not sure what PR commit means. If someone reads the commit title in the CHANGELOG, they would likely get confused and make wrong assumption of this change actually is. I can suggest doc: add note regarding commit message trailers as an alternative, but feel free to chose a different one or to ignore.

Thanks, I've updated the commit message, your suggestion looks good to me 🙂

Also I'm not sure what PR commit means.

Yeah, sorry I am very used to working in repos with PR squash merge strategies and the like so in my mind each PR usually has it's own single commit 🙈

@aduh95 aduh95 added author ready PRs that have at least one approval, no pending requests for changes, and a CI started. commit-queue Add this label to land a pull request using GitHub Actions. labels Jan 24, 2025
@dario-piotrowicz dario-piotrowicz changed the title doc: clarify that PR commit Fixes: are optional doc: add note regarding commit message trailers Jan 25, 2025
@nodejs-github-bot nodejs-github-bot removed the commit-queue Add this label to land a pull request using GitHub Actions. label Jan 26, 2025
@nodejs-github-bot nodejs-github-bot merged commit ce6a628 into nodejs:main Jan 26, 2025
21 checks passed
@nodejs-github-bot
Copy link
Collaborator

Landed in ce6a628

aduh95 added a commit that referenced this pull request Jan 27, 2025
@dario-piotrowicz @anonrig @aduh95
doc: add note regarding commit message trailers
93652f9 
Co-authored-by: Yagiz Nizipli <yagiz@nizipli.com>
Co-authored-by: Antoine du Hamel <duhamelantoine1995@gmail.com>
PR-URL: #56736
Reviewed-By: James M Snell <jasnell@gmail.com>
Reviewed-By: Antoine du Hamel <duhamelantoine1995@gmail.com>
Reviewed-By: Rafael Gonzaga <rafael.nunu@hotmail.com>
Reviewed-By: Luigi Pinca <luigipinca@gmail.com>
@github-actions github-actions bot mentioned this pull request Jan 28, 2025
Merged
aduh95 added a commit that referenced this pull request Jan 30, 2025
@dario-piotrowicz @anonrig @aduh95
doc: add note regarding commit message trailers
38acdb5 
Co-authored-by: Yagiz Nizipli <yagiz@nizipli.com>
Co-authored-by: Antoine du Hamel <duhamelantoine1995@gmail.com>
PR-URL: #56736
Reviewed-By: James M Snell <jasnell@gmail.com>
Reviewed-By: Antoine du Hamel <duhamelantoine1995@gmail.com>
Reviewed-By: Rafael Gonzaga <rafael.nunu@hotmail.com>
Reviewed-By: Luigi Pinca <luigipinca@gmail.com>
hvanness pushed a commit to hvanness/node that referenced this pull request Jan 30, 2025
@dario-piotrowicz @anonrig
@aduh95 @hvanness
doc: add note regarding commit message trailers
d0b37d9 
Co-authored-by: Yagiz Nizipli <yagiz@nizipli.com>
Co-authored-by: Antoine du Hamel <duhamelantoine1995@gmail.com>
PR-URL: nodejs#56736
Reviewed-By: James M Snell <jasnell@gmail.com>
Reviewed-By: Antoine du Hamel <duhamelantoine1995@gmail.com>
Reviewed-By: Rafael Gonzaga <rafael.nunu@hotmail.com>
Reviewed-By: Luigi Pinca <luigipinca@gmail.com>
aduh95 added a commit that referenced this pull request Jan 31, 2025
@dario-piotrowicz @anonrig @aduh95
doc: add note regarding commit message trailers
a9fae78 
Co-authored-by: Yagiz Nizipli <yagiz@nizipli.com>
Co-authored-by: Antoine du Hamel <duhamelantoine1995@gmail.com>
PR-URL: #56736
Reviewed-By: James M Snell <jasnell@gmail.com>
Reviewed-By: Antoine du Hamel <duhamelantoine1995@gmail.com>
Reviewed-By: Rafael Gonzaga <rafael.nunu@hotmail.com>
Reviewed-By: Luigi Pinca <luigipinca@gmail.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@RafaelGSS RafaelGSS RafaelGSS approved these changes

@aduh95 aduh95 aduh95 approved these changes

@lpinca lpinca lpinca approved these changes

@jasnell jasnell jasnell approved these changes

@mcollina mcollina Awaiting requested review from mcollina

@anonrig anonrig Awaiting requested review from anonrig

Assignees
No one assigned
Labels
author ready PRs that have at least one approval, no pending requests for changes, and a CI started. doc Issues and PRs related to the documentations.
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
7 participants
@dario-piotrowicz @nodejs-github-bot @aduh95 @jasnell @lpinca @anonrig @RafaelGSS