-
Notifications
You must be signed in to change notification settings - Fork 29.7k
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
Fix typos in the 9.2.0 changelog. #17062
Conversation
@LJNeon welcome, and thank you for your contribution 🥇 P.S. If you have any question you can also feel free to contact me directly. |
Any objections to fast-track this? |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
-1 The commit messages contain these typos, not solely the changelog. Changing them in here would cause an inconsistency. I believe in the past we've let these typos remain...
cc/ @nodejs/lts Seems better to me to fix them here, we still have the commit hashes to link to the original commit messages. |
Pretty sure I've fixed commit messages in changelogs in the past. I certainly don't see any harm in it. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
- correcting mistakes when we spot them is nice to do
- The original messages cannot be corrected is a technical issue, need not hinder actionable corrections
- source code / links are in tact so the trackability is not lost at all
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM, the link still points to the original commit message.
No "fast-track" as there is debate. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM. Thanks for fixing. The commit message would need to be fixed upon landing
@mscdex are you still -1 on landing this? |
@gibfahn Yes. I also can't find evidence of commit message typos being fixed in the past, unless they were force pushed and there was no later fixup commit. |
I agree with @mscdex, while its unfortunate to have spelling and grammatical errors in the changelog, the commit messages and the changelog should not diverge. We should be very careful to not land commit messages that we don't want to show up in the changelog. |
What's the reasoning for wanting the commit message in the change log to always match that in the commit log down to every typo? I mean, I see why it's preferable on some quasi-aesthetic level, but if one says "mesage" and another says "message", what's the actual harm? Not advocating either way. Truly want to understand the issue so I can weigh the benefits of fixing the typo (clarity; more professional text which rightly or wrongly reflects on the product when people read it) with leaving it (consistency with the commit message, but what's the benefit there?). |
I can think of one tiny benefit: someone can search an exact message title in changelogs to find out what versions have adopted the commit. There may be many other ways to find it out, but someone may know only this simple one. |
Would anyone like to bring this forward to the TSC or should we just close it? (Personally I'm also -1 for reasons outlined in the comment above and will make it explicit if needed.) |
IMHO this falls under the mandate of @nodejs/release, but they might want to punt to the TSC. |
+1 for changing, we've done it on multiple occasions in the past and I'm fine with divergence since users are most likely to use the changelog as a reference. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM
Discussed in the TSC meeting week, open issue in Release repo to ask them to make the decision. |
Due to the concerns in the release group mentioned in nodejs/Release#307 I am going to close this. @LJNeon thanks a lot for your contribution anyway! It is much appreciated and I am sorry that your work could not land. |
See #17061.