doc: CONTRIBUTING.md improvements #19494

jonatack commented at 5:29 PM on July 11, 2020: member

The motivation here was to add a mention of hygienic commits following a discussion today, e.g. something along the lines of:

Make sure each individual commit is hygienic, building successfully on its own without warnings, errors, or regressions, and that all tests pass.

While here, made various fixups. They are optional and can be omitted.

jonatack commented at 5:31 PM on July 11, 2020: member

Friendly ping to Bitcoin's outstanding technical writer, @harding.

in CONTRIBUTING.md:293 in 680e1529cf outdated

 288 | @@ -287,7 +289,7 @@ In general, all pull requests must:
 289 |  
 290 |    - Have a clear use case, fix a demonstrable bug or serve the greater good of
 291 |      the project (for example refactoring for modularisation);
 292 | -  - Be well peer reviewed;
 293 | +  - Be well peer-reviewed;
 294 |    - Have unit tests and functional tests where appropriate;

practicalswift commented at 5:45 PM on July 11, 2020:

  - Have unit tests and functional tests where appropriate;
  - Have unit tests, functional tests and fuzz tests where appropriate;

jonatack commented at 5:48 PM on July 11, 2020:

Good idea.

jonatack commented at 5:54 AM on July 12, 2020:

Added fuzzing in three places.

practicalswift commented at 5:46 PM on July 11, 2020: contributor

Concept ACK

hebasto commented at 5:50 PM on July 11, 2020: member

Concept ACK.

How about adding a requirement to provide a relevant prefix to a commit message as it done for PR title?

DrahtBot added the label Docs on Jul 11, 2020

in CONTRIBUTING.md:11 in 680e1529cf outdated

  12 | -revolves around meritocracy where longer term contributors gain more trust from
  13 | -the developer community. However, some hierarchy is necessary for practical
  14 | -purposes. As such there are repository "maintainers" who are responsible for
  15 | -merging pull requests as well as a "lead maintainer" who is responsible for the
  16 | -release cycle, overall merging, moderation and appointment of maintainers.
  17 | +revolves around meritocracy where long-term contributors gain more trust from

harding commented at 7:05 PM on July 11, 2020:

Nit in the original: should this be "a meritocracy"?

in CONTRIBUTING.md:115 in 680e1529cf outdated

 111 | @@ -113,6 +112,9 @@ In general, [commits should be atomic](https://en.wikipedia.org/wiki/Atomic_comm
 112 |  and diffs should be easy to read. For this reason, do not mix any formatting
 113 |  fixes or code moves with actual code changes.
 114 |  
 115 | +Make sure each individual commit is hygienic, building successfully on its

harding commented at 7:12 PM on July 11, 2020:

Nit: I think introducing an appositive phrase by a comma when that phrase also includes commas can be a bit confusing to read. Also I'd prefer to see the final conjoined object combined with the main list. I suggest: "[...] is hygienic: that it builds successfully on its own without warnings, errors, regressions, or test failures."

in CONTRIBUTING.md:165 in 680e1529cf outdated

 164 | -patch does together with any justification/reasoning. You should include
 165 | -references to any discussions (for example other tickets or mailing list
 166 | -discussions).
 167 | +The body of the pull request should contain sufficient description of *what* the
 168 | +patch does, and even more importantly, *why*, with justification and reasoning.
 169 | +You should include references to any discussions (for example, other tickets or

harding commented at 7:17 PM on July 11, 2020:

Nit in the original: per git grep -i ticket, this is the only occurrence of "ticket" in the repository. OTOH, our documentation and the GitHub interface frequently use "issue". Since this paragraph is being changed anyway, I suggest s/tickets/issues/.

in CONTRIBUTING.md:166 in 680e1529cf outdated

 165 | -references to any discussions (for example other tickets or mailing list
 166 | -discussions).
 167 | +The body of the pull request should contain sufficient description of *what* the
 168 | +patch does, and even more importantly, *why*, with justification and reasoning.
 169 | +You should include references to any discussions (for example, other tickets or
 170 | +mailing list discussions). The PR body should not contain any `@` mentions.

harding commented at 7:30 PM on July 11, 2020:

I think "The PR body should not contain any @ mentions." is a great addition to avoid maintainers or scripts needing to edit things, but I worry it's a bit unclear---"PR body" might be inferred as meaning that no comment on a PR should mention a user. Also, I recently read some wise advice that "and even more importantly, [describe] why [something should be changed], with justification and reasoning." :-)

I suggest: "The description for a new PR should not contain any @ mentions. The PR description will be included in the commit message when the PR is merged and any users mentioned in the description will be annoyingly notified each time a fork of Bitcoin Core copies the merge. Instead, make any username mentions in a subsequent comment to the PR."

in CONTRIBUTING.md:369 in 680e1529cf outdated

 365 | @@ -365,7 +366,7 @@ about:
 366 |  
 367 |    - It may be because of a feature freeze due to an upcoming release. During this time,
 368 |      only bug fixes are taken into consideration. If your pull request is a new feature,
 369 | -    it will not be prioritized until the release is over. Wait for release.
 370 | +    it will not be prioritized until the release is out. Wait for the release.

harding commented at 7:35 PM on July 11, 2020:

Nit: you can avoid the sentence-ending preposition: s/the release is out/after the release/.

harding approved

harding commented at 7:37 PM on July 11, 2020: contributor

LGTM. A few comments, all nits (so ignore anything you think is irrelevant). Thanks!

doc: CONTRIBUTING.md improvements b03697b68e

in CONTRIBUTING.md:83 in 680e1529cf outdated

  79 | @@ -81,8 +80,8 @@ facilitates social contribution, easy testing and peer review.
  80 |  To contribute a patch, the workflow is as follows:
  81 |  
  82 |    1. Fork repository ([only for the first time](https://help.github.com/en/articles/fork-a-repo))
  83 | -  1. Create topic branch
  84 | -  1. Commit patches
  85 | +  2. Create topic branch

practicalswift commented at 9:02 PM on July 11, 2020:

I think the repeated 1. is intentional to let GitHub auto-number the items. Otherwise adding a single new item would require re-numbering all subsequent items manually :)

jonatack force-pushed on Jul 12, 2020

jonatack commented at 6:00 AM on July 12, 2020: member

Thanks @practicalswift, @hebasto and @harding! Excellent feedback, all added to the changes (except commit message prefixes, as I am not sure it's seen as a requirement -- happy to update depending on feedback).

harding commented at 6:43 PM on July 13, 2020: contributor

ACK b03697b68e24bea7a177f84954c93691450d5638 Locally reviewed the word diff.

MarcoFalke commented at 7:06 PM on July 13, 2020: member

ACK b03697b68e24bea7a177f84954c93691450d5638 🚌

<details><summary>Show signature and timestamp</summary>

Signature:

Timestamp of file with hash e10175f73b1b900e2608a1f748db725df303ef08fffac41b83511625d6971544 -

</details>

hebasto approved

hebasto commented at 8:43 PM on July 13, 2020: member

ACK b03697b68e24bea7a177f84954c93691450d5638, I have reviewed the changes and they look OK, I agree they can be merged.

practicalswift commented at 11:08 PM on July 13, 2020: contributor

ACK b03697b68e24bea7a177f84954c93691450d5638

laanwj merged this on Jul 14, 2020

laanwj closed this on Jul 14, 2020

jonatack deleted the branch on Jul 14, 2020

DrahtBot locked this on Feb 15, 2022