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

doc: update onboarding PR landing info #8479

Closed
wants to merge 2 commits into
base: master
from

Conversation

Projects
None yet
5 participants
@Trott
Member

Trott commented Sep 10, 2016

Checklist
  • documentation is changed or added
  • commit message follows commit guidelines
Affected core subsystem(s)

doc

Description of change

Clarify a few items in the onboarding doc about landing a PR. One
addition is to include the optionsl Refs: metadata.

doc: update onboarding PR landing info
Clarify a few items in the onboarding doc about landing a PR. One
addition is to include the optionsl `Refs:` metadata.

@Trott Trott added the doc label Sep 10, 2016

* Only include collaborators who have commented `LGTM`
* Amend the commit description.
* Commits should be of the form `subsystem[,subsystem]: small description\n\nbig description\n\n<metadata>`
* The first line should not exceed 50 characters.

This comment has been minimized.

@lpinca

lpinca Sep 10, 2016

Member

Maybe specify that the description should use the imperative mood.

@lpinca

lpinca Sep 10, 2016

Member

Maybe specify that the description should use the imperative mood.

This comment has been minimized.

@Trott

Trott Sep 10, 2016

Member

I like it, but that gets a little trickier and starts leaking into other docs:

  • Commit message formatting requirements are in CONTRIBUTING.md and belong there, IMO. This doc should just link to that section (except for metadata format, since that's Collaborator-specific).
  • The CONTRIBUTING.md should include the imperative mood requirement. I don't think it does right now. So it will need to be updated too.
  • There may be some discussion around whether using the term imperative mood will be well-understood or is completely accurate. We may want to go with something more like "should start with a present-tense verb". Not sure but there seems to be a risk of bike-shedding.

For those reasons, if it's OK with you, I'd prefer to make the whole adjusting-the-requirements-for-the-commit-message thing be a separate pull request. (Feel free to open it yourself if you wish!)

@Trott

Trott Sep 10, 2016

Member

I like it, but that gets a little trickier and starts leaking into other docs:

  • Commit message formatting requirements are in CONTRIBUTING.md and belong there, IMO. This doc should just link to that section (except for metadata format, since that's Collaborator-specific).
  • The CONTRIBUTING.md should include the imperative mood requirement. I don't think it does right now. So it will need to be updated too.
  • There may be some discussion around whether using the term imperative mood will be well-understood or is completely accurate. We may want to go with something more like "should start with a present-tense verb". Not sure but there seems to be a risk of bike-shedding.

For those reasons, if it's OK with you, I'd prefer to make the whole adjusting-the-requirements-for-the-commit-message thing be a separate pull request. (Feel free to open it yourself if you wish!)

This comment has been minimized.

@lpinca

lpinca Sep 10, 2016

Member

Commit message formatting requirements are in CONTRIBUTING.md and belong there, IMO.

I agree but in this case I would argue that the whole sentence ("The first line should not exceed 50 characters.") belongs to CONTRIBUTING.md.

That said, I'm totally fine with this as is, it's really nitpicking :)

@lpinca

lpinca Sep 10, 2016

Member

Commit message formatting requirements are in CONTRIBUTING.md and belong there, IMO.

I agree but in this case I would argue that the whole sentence ("The first line should not exceed 50 characters.") belongs to CONTRIBUTING.md.

That said, I'm totally fine with this as is, it's really nitpicking :)

Show outdated Hide outdated doc/onboarding.md
* Only include collaborators who have commented `LGTM`.
* Add additional metadata as appropriate:
* `Fixes: <full-issue-url>`
* URL of GitHub issue that the PR fixes.

This comment has been minimized.

@lpinca

lpinca Sep 10, 2016

Member

I would use "Full URL" to avoid confusion with #1234. Same below for Refs:.
Ignore this comment, it is already made clear by Fixes: <full-issue-url>.

@lpinca

lpinca Sep 10, 2016

Member

I would use "Full URL" to avoid confusion with #1234. Same below for Refs:.
Ignore this comment, it is already made clear by Fixes: <full-issue-url>.

This comment has been minimized.

@Trott

Trott Sep 10, 2016

Member

I know you edited this to say to ignore the comment, but I actually rather like the idea and will do it. :-D

@Trott

Trott Sep 10, 2016

Member

I know you edited this to say to ignore the comment, but I actually rather like the idea and will do it. :-D

@mscdex mscdex added the meta label Sep 10, 2016

Show outdated Hide outdated doc/onboarding.md
* `PR-URL: <full-pr-url>`
* `Reviewed-By: human <email>`

This comment has been minimized.

@thefourtheye

thefourtheye Sep 10, 2016

Contributor

s/human/collaborator name/?

@thefourtheye

thefourtheye Sep 10, 2016

Contributor

s/human/collaborator name/?

This comment has been minimized.

@Trott

Trott Sep 10, 2016

Member

Yes, I'll change human to collaborator name. Thanks!

@Trott

Trott Sep 10, 2016

Member

Yes, I'll change human to collaborator name. Thanks!

@lpinca

This comment has been minimized.

Show comment
Hide comment
@lpinca

lpinca Sep 10, 2016

Member

LGTM

Member

lpinca commented Sep 10, 2016

LGTM

@jasnell

This comment has been minimized.

Show comment
Hide comment
@jasnell

jasnell Sep 12, 2016

Member

LGTM

Member

jasnell commented Sep 12, 2016

LGTM

Trott added a commit to Trott/io.js that referenced this pull request Sep 13, 2016

doc: update onboarding PR landing info
Clarify a few items in the onboarding doc about landing a PR. One
addition is to include the optional `Refs:` metadata.

PR-URL: nodejs#8479
Reviewed-By: Luigi Pinca <luigipinca@gmail.com>
Reviewed-By: James M Snell <jasnell@gmail.com>
@Trott

This comment has been minimized.

Show comment
Hide comment
@Trott

Trott Sep 13, 2016

Member

Landed in 4a42ae3

Member

Trott commented Sep 13, 2016

Landed in 4a42ae3

@Trott Trott closed this Sep 13, 2016

Fishrock123 added a commit that referenced this pull request Sep 14, 2016

doc: update onboarding PR landing info
Clarify a few items in the onboarding doc about landing a PR. One
addition is to include the optional `Refs:` metadata.

PR-URL: #8479
Reviewed-By: Luigi Pinca <luigipinca@gmail.com>
Reviewed-By: James M Snell <jasnell@gmail.com>

@lpinca lpinca referenced this pull request Sep 15, 2016

Merged

doc: improve documentation for commit subject line #8546

2 of 2 tasks complete

MylesBorins added a commit that referenced this pull request Sep 30, 2016

doc: update onboarding PR landing info
Clarify a few items in the onboarding doc about landing a PR. One
addition is to include the optional `Refs:` metadata.

PR-URL: #8479
Reviewed-By: Luigi Pinca <luigipinca@gmail.com>
Reviewed-By: James M Snell <jasnell@gmail.com>

MylesBorins added a commit that referenced this pull request Oct 10, 2016

doc: update onboarding PR landing info
Clarify a few items in the onboarding doc about landing a PR. One
addition is to include the optional `Refs:` metadata.

PR-URL: #8479
Reviewed-By: Luigi Pinca <luigipinca@gmail.com>
Reviewed-By: James M Snell <jasnell@gmail.com>

rvagg added a commit that referenced this pull request Oct 18, 2016

doc: update onboarding PR landing info
Clarify a few items in the onboarding doc about landing a PR. One
addition is to include the optional `Refs:` metadata.

PR-URL: #8479
Reviewed-By: Luigi Pinca <luigipinca@gmail.com>
Reviewed-By: James M Snell <jasnell@gmail.com>

MylesBorins added a commit that referenced this pull request Oct 26, 2016

doc: update onboarding PR landing info
Clarify a few items in the onboarding doc about landing a PR. One
addition is to include the optional `Refs:` metadata.

PR-URL: #8479
Reviewed-By: Luigi Pinca <luigipinca@gmail.com>
Reviewed-By: James M Snell <jasnell@gmail.com>

@MylesBorins MylesBorins referenced this pull request Oct 26, 2016

Closed

V4.6.2 proposal #9298

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment