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: Add GitHub pr template #14217

Merged
merged 1 commit into from Sep 28, 2018

Conversation

Projects
None yet
@MarcoFalke
Copy link
Member

commented Sep 13, 2018

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.

@MarcoFalke MarcoFalke added the Docs label Sep 13, 2018

@MarcoFalke MarcoFalke force-pushed the MarcoFalke:Mf1809-docPRtempl branch 5 times, most recently Sep 13, 2018

@ryanofsky

This comment has been minimized.

Copy link
Contributor

commented Sep 13, 2018

No description provided.

Please provide clear motivation for this patch 😃

@PierreRochard

This comment has been minimized.

Copy link
Contributor

commented Sep 13, 2018

ACK

@MarcoFalke MarcoFalke force-pushed the MarcoFalke:Mf1809-docPRtempl branch Sep 14, 2018

.github/PULL_REQUEST_TEMPLATE.md Outdated
bug was fixed.
* Refactoring changes are only accepted if they are required for a feature or
bug fix or otherwise improve developer experience significantly. For
example, most "code style" refactoring changes require an thorough

This comment has been minimized.

Copy link
@ken2812221

ken2812221 Sep 14, 2018

Member

an/a typo

.github/PULL_REQUEST_TEMPLATE.md Outdated
Bitcoin Core user experience or Bitcoin Core developer experience
significantly.

* Any test improvements or new tests that improve coverage are always welcome.

This comment has been minimized.

Copy link
@practicalswift

practicalswift Sep 15, 2018

Member

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

@MarcoFalke MarcoFalke force-pushed the MarcoFalke:Mf1809-docPRtempl branch Sep 15, 2018

.github/PULL_REQUEST_TEMPLATE.md Outdated
Please note that pull requests without a rationale and clear improvement are
closed immediately.

Bitcoin Core has a very thorough review process and even the most trivial

This comment has been minimized.

Copy link
@practicalswift

practicalswift Sep 16, 2018

Member
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.
.github/PULL_REQUEST_TEMPLATE.md Outdated
most "code style" refactoring changes require an thorough explanation why
they are useful, what downsides they have and why they *significantly*
improve developer experience or avoid serious programming bugs. Note that
code style is often a very subjective matter. Unless they are explicitly

This comment has been minimized.

Copy link
@practicalswift

practicalswift Sep 16, 2018

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

@MarcoFalke MarcoFalke force-pushed the MarcoFalke:Mf1809-docPRtempl branch Sep 16, 2018

@ken2812221

This comment has been minimized.

Copy link
Member

commented Sep 19, 2018

utACK c603130f7926b863198d10ac6fd707fc05a78ec6

.github/PULL_REQUEST_TEMPLATE.md Outdated
was fixed.
* Refactoring changes are only accepted if they are required for a feature or
bug fix or otherwise improve developer experience significantly. For example,
most "code style" refactoring changes require an thorough explanation why

This comment has been minimized.

Copy link
@ken2812221

ken2812221 Sep 19, 2018

Member

a/an thorough explanation

@MarcoFalke MarcoFalke force-pushed the MarcoFalke:Mf1809-docPRtempl branch Sep 19, 2018

@ken2812221

This comment has been minimized.

Copy link
Member

commented Sep 19, 2018

re-utACK fa0c680b9a566c3d272e28898777de94fcd0d342

@MarcoFalke

This comment has been minimized.

Copy link
Member Author

commented Sep 20, 2018

Addressed all comment/feedback

.github/PULL_REQUEST_TEMPLATE.md Outdated

Please provide clear motivation for your patch and explain how it improves
Bitcoin Core user experience or Bitcoin Core developer experience
significantly.

This comment has been minimized.

Copy link
@flack

flack Sep 20, 2018

Contributor

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

This comment has been minimized.

Copy link
Member

commented Sep 20, 2018

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
Copy link
Member

left a comment

ACK.

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

.github/PULL_REQUEST_TEMPLATE.md Outdated
@@ -0,0 +1,26 @@
Please note that pull requests without a rationale and clear improvement are

This comment has been minimized.

Copy link
@jnewbery

jnewbery Sep 20, 2018

Member

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

This comment has been minimized.

Copy link
@jnewbery

jnewbery Sep 20, 2018

Member

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

.github/PULL_REQUEST_TEMPLATE.md Outdated
Please note that pull requests without a rationale and clear improvement are
closed immediately.

Bitcoin Core has a thorough review process and even the most trivial change

This comment has been minimized.

Copy link
@jnewbery

jnewbery Sep 20, 2018

Member

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

@MarcoFalke MarcoFalke force-pushed the MarcoFalke:Mf1809-docPRtempl branch Sep 20, 2018

@MarcoFalke

This comment has been minimized.

Copy link
Member Author

commented Sep 20, 2018

Added the test coverage requirement.

@jamesob

This comment has been minimized.

Copy link
Member

commented Sep 20, 2018

reACK

@ryanofsky

This comment has been minimized.

Copy link
Contributor

commented Sep 20, 2018

It seems like this PR is doing two different things:

  1. Adding a pull request template suggesting information to be included with prs.
  2. 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.

@MarcoFalke

This comment has been minimized.

Copy link
Member Author

commented Sep 20, 2018

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

@promag

This comment has been minimized.

Copy link
Member

commented Sep 21, 2018

LGTM

(restarted travis job)

.github/PULL_REQUEST_TEMPLATE.md Outdated

* Any test improvements or new tests that improve coverage are always welcome.
* Features are always welcome, but might be rejected early in the review
process.

This comment has been minimized.

Copy link
@laanwj

laanwj Sep 23, 2018

Member

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

.github/PULL_REQUEST_TEMPLATE.md Outdated
* Bug fixes are most welcome when they come with steps to reproduce or an
explanation of the potential issue as well as reasoning for the way the bug
was fixed.
* Refactoring changes are only accepted if they are required for a feature or

This comment has been minimized.

Copy link
@laanwj
@laanwj

This comment has been minimized.

Copy link
Member

commented Sep 23, 2018

ACK (with nit)

@practicalswift

This comment has been minimized.

Copy link
Member

commented Sep 23, 2018

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

@MarcoFalke MarcoFalke force-pushed the MarcoFalke:Mf1809-docPRtempl branch Sep 23, 2018

@MarcoFalke MarcoFalke force-pushed the MarcoFalke:Mf1809-docPRtempl branch to fae9e84 Sep 23, 2018

@MarcoFalke

This comment has been minimized.

Copy link
Member Author

commented Sep 23, 2018

Addressed all feedback, hopefully.

@practicalswift

This comment has been minimized.

Copy link
Member

commented Sep 23, 2018

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

@MarcoFalke

This comment has been minimized.

Copy link
Member Author

commented Sep 23, 2018

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

@DrahtBot

This comment has been minimized.

Copy link
Contributor

commented Sep 28, 2018

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

@MarcoFalke MarcoFalke merged commit fae9e84 into bitcoin:master Sep 28, 2018

2 checks passed

continuous-integration/appveyor/pr AppVeyor build succeeded
Details
continuous-integration/travis-ci/pr The Travis CI build passed
Details

MarcoFalke added a commit that referenced this pull request Sep 28, 2018

Merge #14217: doc: Add GitHub pr template
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

@MarcoFalke MarcoFalke deleted the MarcoFalke:Mf1809-docPRtempl branch Sep 28, 2018

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.