mirror of
https://github.com/dashpay/dash.git
synced 2024-12-24 11:32:46 +01:00
Merge #19494: doc: CONTRIBUTING.md improvements
b03697b68e24bea7a177f84954c93691450d5638 doc: CONTRIBUTING.md improvements (Jon Atack)
Pull request description:
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.
ACKs for top commit:
harding:
ACK b03697b68e24bea7a177f84954c93691450d5638 Locally reviewed the word diff.
MarcoFalke:
ACK b03697b68e24bea7a177f84954c93691450d5638 🚌
practicalswift:
ACK b03697b68e24bea7a177f84954c93691450d5638
hebasto:
ACK b03697b68e24bea7a177f84954c93691450d5638, I have reviewed the changes and they look OK, I agree they can be merged.
Tree-SHA512: 6fb56219c311d914ec18fcf5d50fdbe3a51e4743a8cace93e348cb4a10c83b6fce631518f1455a1804d1fc81558b235bef58a8be6ccb1a010f46aa4143b1ebf5
This commit is contained in:
Wladimir J. van der Laan
pasta
parent
19ec839e58
commit
34f6a23195
118
CONTRIBUTING.md
118
CONTRIBUTING.md
@ -6,23 +6,24 @@ welcome to contribute towards development in the form of peer review, testing
|
||||
and patches. This document explains the practical process and guidelines for
|
||||
contributing.
|
||||
|
||||
Firstly in terms of structure, there is no particular concept of "Core
|
||||
First, in terms of structure, there is no particular concept of "Dash Core
|
||||
developers" in the sense of privileged people. Open source often naturally
|
||||
revolves around meritocracy where longer term contributors gain more trust from
|
||||
the developer community. However, some hierarchy is necessary for practical
|
||||
purposes. As such there are repository "maintainers" who are responsible for
|
||||
merging pull requests as well as a "lead maintainer" who is responsible for the
|
||||
release cycle, overall merging, moderation and appointment of maintainers.
|
||||
revolves around a meritocracy where contributors earn trust from the developer
|
||||
community over time. Nevertheless, some hierarchy is necessary for practical
|
||||
purposes. As such, there are repository "maintainers" who are responsible for
|
||||
merging pull requests, as well as a "lead maintainer" who is responsible for the
|
||||
release cycle as well as overall merging, moderation and appointment of
|
||||
maintainers.
|
||||
|
||||
Getting Started
|
||||
---------------
|
||||
|
||||
New contributors are very welcome and needed.
|
||||
|
||||
Reviewing and testing is the most effective way you can contribute as a new
|
||||
contributor, and it also will teach you much more about the code and process
|
||||
than opening PRs. Please refer to the section [peer review](#peer-review) later
|
||||
in this document.
|
||||
Reviewing and testing is highly valued and the most effective way you can contribute
|
||||
as a new contributor. It also will teach you much more about the code and
|
||||
process than opening pull requests. Please refer to the [peer review](#peer-review)
|
||||
section below.
|
||||
|
||||
Before you start contributing, familiarize yourself with the Dash Core build
|
||||
system and tests. Refer to the documentation in the repository on how to build
|
||||
@ -48,12 +49,19 @@ you are encouraged to leave a comment if you are planning to work on it. This
|
||||
will help other contributors monitor which issues are actively being addressed
|
||||
and is also an effective way to request assistance if and when you need it.
|
||||
|
||||
Communication Channels
|
||||
----------------------
|
||||
|
||||
Most communication about Dash Core development happens on Discord Server.
|
||||
|
||||
Discussion about codebase improvements happens in GitHub issues and pull
|
||||
requests.
|
||||
|
||||
Contributor Workflow
|
||||
--------------------
|
||||
|
||||
The codebase is maintained using the "contributor workflow" where everyone
|
||||
without exception contributes patch proposals using "pull requests". This
|
||||
without exception contributes patch proposals using "pull requests" (PRs). This
|
||||
facilitates social contribution, easy testing and peer review.
|
||||
|
||||
To contribute a patch, the workflow is as follows:
|
||||
@ -71,6 +79,9 @@ In general, [commits should be atomic](https://en.wikipedia.org/wiki/Atomic_comm
|
||||
and diffs should be easy to read. For this reason, do not mix any formatting
|
||||
fixes or code moves with actual code changes.
|
||||
|
||||
Make sure each individual commit is hygienic: that it builds successfully on its
|
||||
own without warnings, errors, regressions, or test failures.
|
||||
|
||||
Commit messages should be verbose by default consisting of a short subject line
|
||||
(50 chars max), a blank line and detailed explanatory text as separate
|
||||
paragraph(s), unless the title alone is self-explanatory (like "Correct typo
|
||||
@ -82,7 +93,7 @@ If a particular commit references another issue, please add the reference. For
|
||||
example: `refs #1234` or `fixes #4321`. Using the `fixes` or `closes` keywords
|
||||
will cause the corresponding issue to be closed when the pull request is merged.
|
||||
|
||||
Commit messages should never contain any `@` mentions.
|
||||
Commit messages should never contain any `@` mentions (usernames prefixed with "@").
|
||||
|
||||
Please refer to the [Git manual](https://git-scm.com/doc) for more information
|
||||
about Git.
|
||||
@ -129,10 +140,16 @@ Examples:
|
||||
fix(log): fix typo in log message
|
||||
feat(rpc)!: modify gettransaction parameter type
|
||||
|
||||
The body of the pull request should contain enough description about what the
|
||||
patch does together with any justification/reasoning. You should include
|
||||
references to any discussions (for example other tickets or mailing list
|
||||
discussions).
|
||||
The body of the pull request should contain sufficient description of *what* the
|
||||
patch does, and even more importantly, *why*, with justification and reasoning.
|
||||
You should include references to any discussions (for example, other issues or
|
||||
mailing list discussions).
|
||||
|
||||
The description for a new pull request 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 Dash Core copies the merge. Instead, make any username mentions in a
|
||||
subsequent comment to the PR.
|
||||
|
||||
### Translation changes
|
||||
|
||||
@ -168,13 +185,13 @@ before it will be reviewed. The basic squashing workflow is shown below.
|
||||
# Save and quit.
|
||||
git push -f # (force push to GitHub)
|
||||
|
||||
Please update the resulting commit message if needed. It should read as a
|
||||
coherent message. In most cases, this means that you should not just list the
|
||||
interim commits.
|
||||
Please update the resulting commit message, if needed. It should read as a
|
||||
coherent message. In most cases, this means not just listing the interim
|
||||
commits.
|
||||
|
||||
If you have problems with squashing (or other workflows with `git`), you can
|
||||
alternatively enable "Allow edits from maintainers" in the right GitHub
|
||||
sidebar and ask for help in the pull request.
|
||||
If you have problems with squashing or other git workflows, you can enable
|
||||
"Allow edits from maintainers" in the right-hand sidebar of the GitHub web
|
||||
interface and ask for help in the pull request.
|
||||
|
||||
Please refrain from creating several pull requests for the same change.
|
||||
Use the pull request that is already open (or was created earlier) to amend
|
||||
@ -256,8 +273,8 @@ In general, all pull requests must:
|
||||
|
||||
- Have a clear use case, fix a demonstrable bug or serve the greater good of
|
||||
the project (for example refactoring for modularisation);
|
||||
- Be well peer reviewed;
|
||||
- Have unit tests and functional tests where appropriate;
|
||||
- Be well peer-reviewed;
|
||||
- Have unit tests, functional tests, and fuzz tests, where appropriate;
|
||||
- Follow code style guidelines ([C++](doc/developer-notes.md), [functional tests](test/functional/README.md));
|
||||
- Not break the existing test suite;
|
||||
- Where bugs are fixed, where possible, there should be unit tests
|
||||
@ -284,41 +301,40 @@ spread out over GitHub, mailing list and IRC discussions).
|
||||
#### Conceptual Review
|
||||
|
||||
A review can be a conceptual review, where the reviewer leaves a comment
|
||||
* `Concept (N)ACK`, meaning "I do (not) agree in the general goal of this pull
|
||||
* `Concept (N)ACK`, meaning "I do (not) agree with the general goal of this pull
|
||||
request",
|
||||
* `Approach (N)ACK`, meaning `Concept ACK`, but "I do (not) agree with the
|
||||
approach of this change".
|
||||
|
||||
#### Code Review
|
||||
|
||||
After conceptual agreement on the change, code review can be provided. It is
|
||||
starting with `ACK BRANCH_COMMIT`, where `BRANCH_COMMIT` is the top of the
|
||||
topic branch. The review is followed by a description of how the reviewer did
|
||||
the review. The following
|
||||
language is used within pull-request comments:
|
||||
After conceptual agreement on the change, code review can be provided. A review
|
||||
begins with `ACK BRANCH_COMMIT`, where `BRANCH_COMMIT` is the top of the PR
|
||||
branch, followed by a description of how the reviewer did the review. The
|
||||
following language is used within pull request comments:
|
||||
|
||||
- (t)ACK means "I have tested the code and I agree it should be merged", involving
|
||||
change-specific manual testing in addition to running the unit and functional
|
||||
tests, and in case it is not obvious how the manual testing was done, it should
|
||||
be described;
|
||||
- (t)ACK means "I have tested the code and I agree it should be merged",
|
||||
involving change-specific manual testing in
|
||||
addition to running the unit, functional, or fuzz tests, and in case it is
|
||||
not obvious how the manual testing was done, it should be described;
|
||||
- NACK means "I disagree this should be merged", and must be accompanied by
|
||||
sound technical justification (or in certain cases of copyright/patent/licensing
|
||||
issues, legal justification). NACKs without accompanying reasoning may be
|
||||
disregarded;
|
||||
- utACK means "I have not tested the code, but I have reviewed it and it looks
|
||||
OK, I agree it can be merged";
|
||||
- Nit refers to trivial, often non-blocking issues.
|
||||
- A "nit" refers to a trivial, often non-blocking issue.
|
||||
|
||||
Project maintainers reserve the right to weigh the opinions of peer reviewers
|
||||
using common sense judgement and also may weight based on meritocracy: Those
|
||||
that have demonstrated a deeper commitment and understanding towards the project
|
||||
(over time) or have clear domain expertise may naturally have more weight, as
|
||||
one would expect in all walks of life.
|
||||
using common sense judgement and may also weigh based on merit. Reviewers that
|
||||
have demonstrated a deeper commitment and understanding of the project over time
|
||||
or who have clear domain expertise may naturally have more weight, as one would
|
||||
expect in all walks of life.
|
||||
|
||||
Where a patch set affects consensus critical code, the bar will be set much
|
||||
Where a patch set affects consensus-critical code, the bar will be much
|
||||
higher in terms of discussion and peer review requirements, keeping in mind that
|
||||
mistakes could be very costly to the wider community. This includes refactoring
|
||||
of consensus critical code.
|
||||
of consensus-critical code.
|
||||
|
||||
Where a patch set proposes to change the Dash consensus, it must have been
|
||||
discussed extensively on the mailing list and IRC, be accompanied by a widely
|
||||
@ -353,7 +369,7 @@ of reasons for this, some of which you can do something about:
|
||||
|
||||
- It may be because of a feature freeze due to an upcoming release. During this time,
|
||||
only bug fixes are taken into consideration. If your pull request is a new feature,
|
||||
it will not be prioritized until the release is over. Wait for release.
|
||||
it will not be prioritized until after the release. Wait for the release.
|
||||
- It may be because the changes you are suggesting do not appeal to people. Rather than
|
||||
nits and critique, which require effort and means they care enough to spend time on your
|
||||
contribution, thundering silence is a good sign of widespread (mild) dislike of a given change
|
||||
@ -363,16 +379,18 @@ of reasons for this, some of which you can do something about:
|
||||
[developer notes](doc/developer-notes.md), is dangerous or insecure, is messily written, etc.
|
||||
Identify and address any of the issues you find. Then ask e.g. on the forum or on a community
|
||||
discord if someone could give their opinion on the concept itself.
|
||||
- It may be because your code is too complex for all but a few people. And those people
|
||||
- It may be because your code is too complex for all but a few people, and those people
|
||||
may not have realized your pull request even exists. A great way to find people who
|
||||
are qualified and care about the code you are touching is the
|
||||
[Git Blame feature](https://help.github.com/articles/tracing-changes-in-a-file/). Simply
|
||||
find the person touching the code you are touching before you and see if you can find
|
||||
them and give them a nudge. Don't be incessant about the nudging though.
|
||||
look up who last modified the code you are changing and see if you can find
|
||||
them and give them a nudge. Don't be incessant about the nudging, though.
|
||||
- Finally, if all else fails, ask on discord or elsewhere for someone to give your pull request
|
||||
a look. If you think you've been waiting an unreasonably long amount of time (month+) for
|
||||
no particular reason (few lines changed, etc), this is totally fine. Try to return the favor
|
||||
when someone else is asking for feedback on their code, and universe balances out.
|
||||
a look. If you think you've been waiting for an unreasonably long time (say,
|
||||
more than a month) for no particular reason (a few lines changed, etc.),
|
||||
this is totally fine. Try to return the favor when someone else is asking
|
||||
for feedback on their code, and the universe balances out.
|
||||
- Remember that the best thing you can do while waiting is give review to others!
|
||||
|
||||
|
||||
Backporting
|
||||
@ -381,11 +399,11 @@ Backporting
|
||||
Security and bug fixes can be backported from `master` to release
|
||||
branches.
|
||||
If the backport is non-trivial, it may be appropriate to open an
|
||||
additional PR, to backport the change, only after the original PR
|
||||
additional PR to backport the change, but only after the original PR
|
||||
has been merged.
|
||||
Otherwise, backports will be done in batches and
|
||||
the maintainers will use the proper `Needs backport (...)` labels
|
||||
when needed (the original author does not need to worry).
|
||||
when needed (the original author does not need to worry about it).
|
||||
|
||||
A backport should contain the following metadata in the commit body:
|
||||
|
||||
|
||||
Loading…
Reference in New Issue
Block a user