Bitcoin Core has a very thorough review process and even the most trivial change needs to pass a lot of eyes and requires non-zero or even substantial time effort to review. There is a huge lack of active reviewers on the project, so patches often sit for a long time.
Authors should provide clear motivation for patches and explain how it improves Bitcoin Core user experience or Bitcoin Core developer experience significantly.
No description provided.
Please provide clear motivation for this patch :smiley:
16+ * Bug fixes are most welcome when they come with steps to reproduce or an
17+ explanation of the potential issue as well as reasoning for the way the
18+ bug was fixed.
19+ * Refactoring changes are only accepted if they are required for a feature or
20+ bug fix or otherwise improve developer experience significantly. For
21+ example, most "code style" refactoring changes require an thorough
8+
9+Please provide clear motivation for your patch and explain how it improves
10+Bitcoin Core user experience or Bitcoin Core developer experience
11+significantly.
12+
13+ * Any test improvements or new tests that improve coverage are always welcome.
Markdown nit:
02018-09-15 11:00:30 mdl(pr=14217): .github/PULL_REQUEST_TEMPLATE.md:13: MD006 Consider starting bulleted lists at the beginning of the line
0@@ -0,0 +1,26 @@
1+Please note that pull requests without a rationale and clear improvement are
2+closed immediately.
3+
4+Bitcoin Core has a very thorough review process and even the most trivial
02018-09-15 17:29:22 proselint(pr=14217): .github/PULL_REQUEST_TEMPLATE.md:4:20: weasel_words.very Substitute 'damn' every time you're inclined to write 'very;' your editor will delete it and the writing will be just as it should be. Found once elsewhere.
19+* Refactoring changes are only accepted if they are required for a feature or
20+ bug fix or otherwise improve developer experience significantly. For example,
21+ most "code style" refactoring changes require an thorough explanation why
22+ they are useful, what downsides they have and why they *significantly*
23+ improve developer experience or avoid serious programming bugs. Note that
24+ code style is often a very subjective matter. Unless they are explicitly
02018-09-15 17:29:22 proselint(pr=14217): .github/PULL_REQUEST_TEMPLATE.md:4:20: weasel_words.very Substitute 'damn' every time you're inclined to write 'very;' your editor will delete it and the writing will be just as it should be. Found once elsewhere.
16+* Bug fixes are most welcome when they come with steps to reproduce or an
17+ explanation of the potential issue as well as reasoning for the way the bug
18+ was fixed.
19+* Refactoring changes are only accepted if they are required for a feature or
20+ bug fix or otherwise improve developer experience significantly. For example,
21+ most "code style" refactoring changes require an thorough explanation why
6+effort to review. There is a huge lack of active reviewers on the project, so
7+patches often sit for a long time.
8+
9+Please provide clear motivation for your patch and explain how it improves
10+Bitcoin Core user experience or Bitcoin Core developer experience
11+significantly.
Not sure if I’m reading this right, but is the intention to say that a change needs to
or is it more that a change has to
Because the way I read it is that significantly applies to both terms, but I imagine that significantly is only supposed to apply to the second term. Because why would you turn down a papercut fix that smoothes the UX a little?
IOW, maybe this could be rephrased to be less ambiguous
ACK https://github.com/bitcoin/bitcoin/pull/14217/commits/fa0c680b9a566c3d272e28898777de94fcd0d342
It might be worth saying something about test coverage. I’m not sure what our established policy is here, but maybe something like:
0* All changes should have accompanying unit tests (see `src/test/`) or functional tests (see `test/`).
1 PR authors should note which tests cover modified code. If no tests exist for a region of
2 modified code, new tests should accompany the change.
0@@ -0,0 +1,26 @@
1+Please note that pull requests without a rationale and clear improvement are
0@@ -0,0 +1,26 @@
1+Please note that pull requests without a rationale and clear improvement are
2+closed immediately.
3+
4+Bitcoin Core has a thorough review process and even the most trivial change
ACK.
I’ve offered my preferred colour of paint for your shed. Feel free to use it or not.
It seems like this PR is doing two different things:
I personally only like the first thing and not the second thing, but don’t want to interfere if this is how maintainers want to handle PRs.
LGTM
(restarted travis job)
5+Bitcoin Core user experience or Bitcoin Core developer experience
6+significantly.
7+
8+* Any test improvements or new tests that improve coverage are always welcome.
9+* Features are always welcome, but might be rejected early in the review
10+ process.
9+* Features are always welcome, but might be rejected early in the review
10+ process.
11+* Bug fixes are most welcome when they come with steps to reproduce or an
12+ explanation of the potential issue as well as reasoning for the way the bug
13+ was fixed.
14+* Refactoring changes are only accepted if they are required for a feature or
Since this guide will influence the type of contributions we get: shouldn’t we mention security work?
Personally I’m much more excited to see a PR submitted which strengthens the long term security of the project in some form compared to when I see a PR submitted which implements some shiny new feature :-)
The formulation “Features are always welcome […]” contrasted to “Bug fixes are most welcome when […]” sounds almost like we’re encouraging new users to help expand the feature set of Core :-)
Something that might be worth mentioning is the special security requirements/trade-offs that apply to changes to the consensus critical parts of the code base. Perhaps also mentioning the rough boundaries of the consensus critical code.
More specifically it might be worth mentioning why a change of type A would be an obvious ACK if applied to say the Qt part of the code base while it would be an obvious NACK if applied to the consensus critical part of the code base.
Basically to drive home the point that we’re extremely focused on correctness and security for good reasons :-)
| Coverage | Change (pull 14217) | Reference (master) |
|---|---|---|
| Lines | -0.0066 % | 87.0361 % |
| Functions | +0.0618 % | 84.1130 % |
| Branches | -0.0180 % | 51.5451 % |