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

Fix typos in the 9.2.0 changelog. #17062

Closed
wants to merge 1 commit into from
Closed

Fix typos in the 9.2.0 changelog. #17062

wants to merge 1 commit into from

Conversation

LJNeon
Copy link

@LJNeon LJNeon commented Nov 15, 2017

See #17061.

@nodejs-github-bot nodejs-github-bot added the doc Issues and PRs related to the documentations. label Nov 15, 2017
@refack
Copy link
Contributor

refack commented Nov 15, 2017

@LJNeon 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 question you can also feel free to contact me directly.

@refack refack added the fast-track PRs that do not need to wait for 48 hours to land. label Nov 15, 2017
@refack refack self-assigned this Nov 15, 2017
@refack
Copy link
Contributor

refack commented Nov 15, 2017

Any objections to fast-track this?

Copy link
Contributor

@mscdex mscdex left a 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...

@gibfahn
Copy link
Member

gibfahn commented Nov 15, 2017

cc/ @nodejs/lts

Seems better to me to fix them here, we still have the commit hashes to link to the original commit messages.

@Fishrock123
Copy link
Contributor

Fishrock123 commented Nov 16, 2017

Pretty sure I've fixed commit messages in changelogs in the past. I certainly don't see any harm in it.

Copy link
Member

@gireeshpunathil gireeshpunathil left a 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

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.

LGTM, the link still points to the original commit message.

@refack refack removed the fast-track PRs that do not need to wait for 48 hours to land. label Nov 16, 2017
@refack
Copy link
Contributor

refack commented Nov 16, 2017

No "fast-track" as there is debate.

Copy link
Contributor

@evanlucas evanlucas left a 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

@gibfahn
Copy link
Member

gibfahn commented Nov 16, 2017

@mscdex are you still -1 on landing this?

@mscdex
Copy link
Contributor

mscdex commented Nov 16, 2017

@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.

@sam-github
Copy link
Contributor

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.

@richardlau richardlau mentioned this pull request Nov 18, 2017
3 tasks
@Trott
Copy link
Member

Trott commented Nov 18, 2017

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?).

@vsemozhetbyt
Copy link
Contributor

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.

@apapirovski
Copy link
Member

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.)

@refack
Copy link
Contributor

refack commented Dec 9, 2017

IMHO this falls under the mandate of @nodejs/release, but they might want to punt to the TSC.
Either way I think this should be codified.

@rvagg
Copy link
Member

rvagg commented Feb 7, 2018

+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.

Copy link
Member

@mcollina mcollina left a comment

Choose a reason for hiding this comment

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

LGTM

@mhdawson mhdawson added the release-agenda Issues and PRs to discuss during the meetings of the Release team. label Feb 7, 2018
@mhdawson
Copy link
Member

mhdawson commented Feb 7, 2018

Discussed in the TSC meeting week, open issue in Release repo to ask them to make the decision.

@BridgeAR
Copy link
Member

BridgeAR commented Mar 4, 2018

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.

@BridgeAR BridgeAR closed this Mar 4, 2018
@refack refack removed their assignment Oct 12, 2018
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
doc Issues and PRs related to the documentations. release-agenda Issues and PRs to discuss during the meetings of the Release team.
Projects
None yet
Development

Successfully merging this pull request may close these issues.