doc: Add GitHub pr template by maflcko · Pull Request #14217 · bitcoin/bitcoin

maflcko · 2018-09-13T22:42:22Z

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.

ryanofsky · 2018-09-13T23:08:12Z

No description provided.

Please provide clear motivation for this patch 😃

PierreRochard · 2018-09-13T23:15:36Z

ACK

ken2812221 · 2018-09-14T01:25:42Z

practicalswift · 2018-09-15T14:47:07Z

Markdown nit:

2018-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

practicalswift · 2018-09-16T13:41:43Z

2018-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.

practicalswift · 2018-09-16T13:41:54Z

2018-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.

ken2812221 · 2018-09-19T01:26:34Z

utACK c603130f7926b863198d10ac6fd707fc05a78ec6

ken2812221 · 2018-09-19T01:28:43Z

a/an thorough explanation

ken2812221 · 2018-09-19T04:47:30Z

re-utACK fa0c680b9a566c3d272e28898777de94fcd0d342

maflcko · 2018-09-20T17:08:34Z

Addressed all comment/feedback

flack · 2018-09-20T19:42:29Z

Not sure if I'm reading this right, but is the intention to say that a change needs to

significantly improve user experience
or

significantly improve developer experience

or is it more that a change has to

improve user experience
or

significantly improve developer experience

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

jamesob · 2018-09-20T19:44:11Z

ACK fa0c680

It might be worth saying something about test coverage. I'm not sure what our established policy is here, but maybe something like:

* All changes should have accompanying unit tests (see `src/test/`) or functional tests (see `test/`). 
  PR authors should note which tests cover modified code. If no tests exist for a region of 
  modified code, new tests should accompany the change.

jnewbery

ACK.

I've offered my preferred colour of paint for your shed. Feel free to use it or not.

jnewbery · 2018-09-20T19:44:23Z

Please note that 'please note that' is conent-free in almost all circumstances, and is usually better left out. :)

I'd also change 'are' to 'may be'

jnewbery · 2018-09-20T19:44:52Z

I'd move this to the end. The next paragraph 'Please provide clear motivation...' is more important.

maflcko · 2018-09-20T20:04:38Z

Added the test coverage requirement.

jamesob · 2018-09-20T21:01:06Z

reACK

ryanofsky · 2018-09-20T21:16:05Z

It seems like this PR is doing two different things:

Adding a pull request template suggesting information to be included with prs.
Introducing a new policy ("Refactoring changes are only accepted...") of rejecting refactoring changes instead of just treating them with low priority.

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.

maflcko · 2018-09-20T21:24:33Z

@ryanofsky The policy was introduced about a year ago: 91c39e3

promag · 2018-09-21T13:14:16Z

LGTM

(restarted travis job)

laanwj · 2018-09-23T11:46:48Z

"are always welcome, but might be rejected" is a bit awkward—I think it should note why features are rejected. For example, to keep the project from expanding in scope, as it dilutes focus and increases maintenance burden. Or that we don't want to add dependencies. Contributors should consider first whether it isn't possible to build system outside of the bitcoin core codebase.

laanwj · 2018-09-23T11:47:00Z

laanwj · 2018-09-23T11:47:40Z

ACK (with nit)

practicalswift · 2018-09-23T12:14:34Z

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

maflcko · 2018-09-23T12:32:45Z

Addressed all feedback, hopefully.

practicalswift · 2018-09-23T12:51:21Z

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

maflcko · 2018-09-23T14:05:02Z

@practicalswift I believe this is covered in the contributions guideline: https://github.com/bitcoin/bitcoin/blob/master/CONTRIBUTING.md#peer-review?

DrahtBot · 2018-09-28T11:06:34Z

Coverage	Change (pull 14217)	Reference (master)
Lines	-0.0066 %	87.0361 %
Functions	+0.0618 %	84.1130 %
Branches	-0.0180 %	51.5451 %

fae9e84 doc: Add GitHub pr template (MarcoFalke) Pull request description: 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. Tree-SHA512: 83b379d75934089f13ba173e6ec847845f954f10f83e406ef9722836aa093170c612b4214f22cd5939d59f66a50f5d0e52aa0059423e8e7261bb176f1c546a08

maflcko added the Docs label Sep 13, 2018

maflcko force-pushed the Mf1809-docPRtempl branch 5 times, most recently from b632a20 to 77dc435 Compare September 13, 2018 22:52

maflcko force-pushed the Mf1809-docPRtempl branch from 77dc435 to ecc80fc Compare September 14, 2018 01:18

ken2812221 reviewed Sep 14, 2018

View reviewed changes

Comment thread .github/PULL_REQUEST_TEMPLATE.md Outdated

ken2812221 Sep 14, 2018

Copy link
Copy Markdown

Contributor

Choose a reason for hiding this comment

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

an/a typo

practicalswift reviewed Sep 15, 2018

View reviewed changes

maflcko force-pushed the Mf1809-docPRtempl branch from ecc80fc to 1e6aa1d Compare September 15, 2018 14:55

practicalswift reviewed Sep 16, 2018

View reviewed changes

maflcko force-pushed the Mf1809-docPRtempl branch from 1e6aa1d to c603130 Compare September 16, 2018 13:48

ken2812221 reviewed Sep 19, 2018

View reviewed changes

Comment thread .github/PULL_REQUEST_TEMPLATE.md Outdated

ken2812221 Sep 19, 2018

Copy link
Copy Markdown

Contributor

Choose a reason for hiding this comment

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

a/an thorough explanation

maflcko force-pushed the Mf1809-docPRtempl branch from c603130 to fa0c680 Compare September 19, 2018 02:43

flack reviewed Sep 20, 2018

View reviewed changes

jnewbery reviewed Sep 20, 2018

View reviewed changes

maflcko force-pushed the Mf1809-docPRtempl branch from fa0c680 to fadc2a9 Compare September 20, 2018 20:03

ryanofsky mentioned this pull request Sep 20, 2018

Improve workflow for maintainers PierreRochard/bitcoin-acks#75

Open

laanwj reviewed Sep 23, 2018

View reviewed changes

Comment thread .github/PULL_REQUEST_TEMPLATE.md Outdated

laanwj Sep 23, 2018

Copy link
Copy Markdown

Member

Choose a reason for hiding this comment

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

👍

maflcko force-pushed the Mf1809-docPRtempl branch from fadc2a9 to fa6f85a Compare September 23, 2018 12:26

doc: Add GitHub pr template

fae9e84

maflcko force-pushed the Mf1809-docPRtempl branch from fa6f85a to fae9e84 Compare September 23, 2018 12:31

practicalswift mentioned this pull request Sep 24, 2018

Tests: enforce critical class instance attributes in functional tests, fix segwit test specificity #14305

Merged

maflcko merged commit fae9e84 into bitcoin:master Sep 28, 2018

maflcko deleted the Mf1809-docPRtempl branch September 28, 2018 11:56

Bushstar mentioned this pull request Oct 11, 2018

Updates from bitcoin/master FeatherCoin/Feathercoin#366

Merged

bitcoin locked as resolved and limited conversation to collaborators Sep 8, 2021

Uh oh!

Conversation

maflcko commented Sep 13, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

ryanofsky commented Sep 13, 2018

Uh oh!

PierreRochard commented Sep 13, 2018

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

ken2812221 commented Sep 19, 2018

Uh oh!

Choose a reason for hiding this comment

Uh oh!

ken2812221 commented Sep 19, 2018

Uh oh!

maflcko commented Sep 20, 2018

Uh oh!

Choose a reason for hiding this comment

Uh oh!

jamesob commented Sep 20, 2018

Uh oh!

jnewbery left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

maflcko commented Sep 20, 2018

Uh oh!

jamesob commented Sep 20, 2018

Uh oh!

ryanofsky commented Sep 20, 2018

Uh oh!

maflcko commented Sep 20, 2018

Uh oh!

promag commented Sep 21, 2018

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

laanwj commented Sep 23, 2018

Uh oh!

practicalswift commented Sep 23, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

maflcko commented Sep 23, 2018

Uh oh!

practicalswift commented Sep 23, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

maflcko commented Sep 23, 2018

Uh oh!

DrahtBot commented Sep 28, 2018

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

11 participants

maflcko commented Sep 13, 2018 •

edited

Loading

practicalswift commented Sep 23, 2018 •

edited

Loading

practicalswift commented Sep 23, 2018 •

edited

Loading