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

doc: update onboarding PR landing info #8479

Closed
wants to merge 2 commits into from

Conversation

Trott
Copy link
Member

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

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 Issues and PRs related to the documentations. 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.
Copy link
Member

Choose a reason for hiding this comment

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

Maybe specify that the description should use the imperative mood.

Copy link
Member Author

@Trott Trott Sep 10, 2016

Choose a reason for hiding this comment

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

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

Copy link
Member

Choose a reason for hiding this comment

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

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

@mscdex mscdex added the meta Issues and PRs related to the general management of the project. label Sep 10, 2016
* `PR-URL: <full-pr-url>`
* `Reviewed-By: human <email>`
Copy link
Contributor

Choose a reason for hiding this comment

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

s/human/collaborator name/?

Copy link
Member Author

Choose a reason for hiding this comment

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

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

@lpinca
Copy link
Member

lpinca commented Sep 10, 2016

LGTM

1 similar comment
@jasnell
Copy link
Member

jasnell commented Sep 12, 2016

LGTM

Trott added a commit to Trott/io.js that referenced this pull request Sep 13, 2016
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
Copy link
Member Author

Trott commented Sep 13, 2016

Landed in 4a42ae3

@Trott Trott closed this Sep 13, 2016
Fishrock123 pushed a commit that referenced this pull request Sep 14, 2016
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 pushed a commit that referenced this pull request Sep 30, 2016
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 pushed a commit that referenced this pull request Oct 10, 2016
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 pushed a commit that referenced this pull request Oct 18, 2016
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 pushed a commit that referenced this pull request Oct 26, 2016
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 mentioned this pull request Oct 26, 2016
@Trott Trott deleted the onboarding-refs branch January 13, 2022 22:44
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. meta Issues and PRs related to the general management of the project.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants