-
Notifications
You must be signed in to change notification settings - Fork 1.6k
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
Add small pr guidelines #2977
Add small pr guidelines #2977
Conversation
Codecov Report
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This looks great, added grammar nits - plus I would add one more section somewhere :
Implementing the full functionality sometimes makes a lot of sense, so that you can get feedback regarding the design while implementing it, also, the maintainers can try it out and test it, get a feel for it. We can mark these "massive" PRs "prototype PR" or "master PR" that will broken down into smaller PRs that can refer back to the master PR. If you keep the master PR rebased as the smaller pieces get merged, finally the master PR can be merged as well. See for example #2917.
WDYT?
code review changes. Co-Authored-By: Balint Pato <[email protected]>
We would want to encourage sending a With that said, i do understand scenarios like this #2978 I feel it might send conflicting messages but open for discussion. |
addressed review comments as we discussed.
Fixes #2812
Description
I this PR, add guidelines for keeping PRs small,
User facing changes
Next PRs.
Explain how one should address only one bug in a PR.
Submitter Checklist
These are the criteria that every PR should meet, please check them off as you
review them:
See the contribution guide for more details.
Double check this list of stuff that's easy to miss:
examples
dir, please copy them tointegration/examples
integration/examples
dir, should be tested in integration testReviewer Notes
Release Notes
Describe any user facing changes here so maintainer can include it in the release notes, or delete this block.