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

src: fix typos in node_lttng_provider.h #11723

Closed
wants to merge 1 commit into from

Conversation

bf4
Copy link
Contributor

@bf4 bf4 commented Mar 6, 2017

Follows #11189

To be considered a breaking change per #11189 (comment)

@nodejs-github-bot nodejs-github-bot added the c++ Issues and PRs that require attention from people who are familiar with C++. label Mar 6, 2017
@jasnell
Copy link
Member

jasnell commented Mar 6, 2017

Can you format the first line of the commit message in accordance with the guidelines in the contribution guide?

@addaleax addaleax added the lib / src Issues and PRs related to general changes in the lib or src directory. label Mar 6, 2017
@mscdex mscdex added semver-major PRs that contain breaking changes and should be released in the next major version. and removed lib / src Issues and PRs related to general changes in the lib or src directory. labels Mar 7, 2017
@bf4
Copy link
Contributor Author

bf4 commented Mar 7, 2017

@jasnell

Can you format the first line of the commit message in accordance with the guidelines in the contribution guide?

I find the contribution guidelines kind of hard to parse.

The first line should be 50 characters or less and contain a short description of the change. All words in the description should be in lowercase with the exception of proper nouns, acronyms, and the ones that refer to code, like function/variable names. The description should be prefixed with the name of the changed subsystem and start with an imperative verb. Example: "net: add localAddress and localPort to Socket"

Are you asking me to format the commit message in lowercase? The PR description in lowercase? Or what subsystem would you call this? src: correct typos

If your patch fixes an open issue, you can add a reference to it at the end of the log. Use the Fixes: prefix and the full issue URL. For other references use Refs:. For example:

Should I add to the body of the commit message Refs: https://github.com/nodejs/node/pull/11189#discussion_r99509433

@lpinca
Copy link
Member

lpinca commented Mar 7, 2017

Are you asking me to format the commit message in lowercase?

Yes. Something like src: fix typos in node_lttng_provider.h should work.
You can detail what typos have been fixed in the commit body but I think it's not necessary.
I would also not include the Refs: metadata as that comment does not provide any additional useful information imo, it doesn't harm though.

@bf4 bf4 force-pushed the correct_typos_breaking_change branch from 6617cb0 to cfe7339 Compare March 7, 2017 15:40
@bf4 bf4 changed the title Breaking change, typo "s/Unkown/Unknown GC Type" src: fix typos in node_lttng_provider.h Mar 7, 2017
@bf4
Copy link
Contributor Author

bf4 commented Mar 7, 2017

@jasnell @lpinca Done

@jasnell
Copy link
Member

jasnell commented Mar 8, 2017

Copy link
Member

@addaleax addaleax left a comment

Choose a reason for hiding this comment

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

@addaleax
Copy link
Member

Landed in 1125c8a, thanks for the PR!

@addaleax addaleax closed this Mar 10, 2017
addaleax pushed a commit that referenced this pull request Mar 10, 2017
Is a semver-breaking change

Refs: #11189 (comment)
PR-URL: #11723
Reviewed-By: James M Snell <jasnell@gmail.com>
Reviewed-By: Anna Henningsen <anna@addaleax.net>
Reviewed-By: Luigi Pinca <luigipinca@gmail.com>
Reviewed-By: Colin Ihrig <cjihrig@gmail.com>
@bf4 bf4 deleted the correct_typos_breaking_change branch March 13, 2017 01:25
jungx098 pushed a commit to jungx098/node that referenced this pull request Mar 21, 2017
Is a semver-breaking change

Refs: nodejs#11189 (comment)
PR-URL: nodejs#11723
Reviewed-By: James M Snell <jasnell@gmail.com>
Reviewed-By: Anna Henningsen <anna@addaleax.net>
Reviewed-By: Luigi Pinca <luigipinca@gmail.com>
Reviewed-By: Colin Ihrig <cjihrig@gmail.com>
@jasnell jasnell mentioned this pull request Apr 4, 2017
gibfahn pushed a commit that referenced this pull request Apr 22, 2017
- Can now link to 'Commit Guidelines' from pull requests
- Breaks up commit requirements and recommendations

PR-URL: #11732
Refs: #11723 (comment)
Reviewed-By: Gibson Fahnestock <gibfahn@gmail.com>
Reviewed-By: Sam Roberts <vieuxtech@gmail.com>
evanlucas pushed a commit that referenced this pull request Apr 25, 2017
- Can now link to 'Commit Guidelines' from pull requests
- Breaks up commit requirements and recommendations

PR-URL: #11732
Refs: #11723 (comment)
Reviewed-By: Gibson Fahnestock <gibfahn@gmail.com>
Reviewed-By: Sam Roberts <vieuxtech@gmail.com>
evanlucas pushed a commit that referenced this pull request May 1, 2017
- Can now link to 'Commit Guidelines' from pull requests
- Breaks up commit requirements and recommendations

PR-URL: #11732
Refs: #11723 (comment)
Reviewed-By: Gibson Fahnestock <gibfahn@gmail.com>
Reviewed-By: Sam Roberts <vieuxtech@gmail.com>
evanlucas pushed a commit that referenced this pull request May 2, 2017
- Can now link to 'Commit Guidelines' from pull requests
- Breaks up commit requirements and recommendations

PR-URL: #11732
Refs: #11723 (comment)
Reviewed-By: Gibson Fahnestock <gibfahn@gmail.com>
Reviewed-By: Sam Roberts <vieuxtech@gmail.com>
gibfahn pushed a commit that referenced this pull request May 16, 2017
- Can now link to 'Commit Guidelines' from pull requests
- Breaks up commit requirements and recommendations

PR-URL: #11732
Refs: #11723 (comment)
Reviewed-By: Gibson Fahnestock <gibfahn@gmail.com>
Reviewed-By: Sam Roberts <vieuxtech@gmail.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
c++ Issues and PRs that require attention from people who are familiar with C++. semver-major PRs that contain breaking changes and should be released in the next major version.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

7 participants