[MRG+2] DOC: add issue and pull request template #6411
Conversation
| @@ -0,0 +1,10 @@ | |||
| Please try to adhere to the guidelines below as much as possible when submitting your pull request. | |||
|
|
|||
| - [ ] Please add `[WIP]` to the beginning of your pull request title if you are still working on it or it is not ready for review. | |||
There was a problem hiding this comment.
Rewriting:
- [ ] Please prefix the title of your pull request with `[MRG]` if it the contribution is complete and should be subjected to detail review. Otherwise:
- [ ] Incomplete contributions should be prefixed `[WIP]` to indicate a work in progress (and change it to `[MRG]` when it matures). WIPs may be useful to: indicate you are working on something to avoid duplicated work; request broad review of functionality or API; or seek collaborators. WIPs often benefit from the inclusion of a [task list](https://github.com/blog/1375-task-lists-in-gfm-issues-pulls-comments) in the PR description.
There was a problem hiding this comment.
this is a much better way of putting it, thanks. Is the info about [MRG+x] unnecessary? I found that difficult to describe.
There was a problem hiding this comment.
Yes, I think it's unnecessary.
|
This looks great – thanks for putting it together! |
| Please try to adhere to the guidelines below as much as possible when submitting your issue. | ||
| - [ ] Verify that your issue is not being currently addressed by other [open issues](https://github.com/scikit-learn/scikit-learn/issues) or [pull requests](https://github.com/scikit-learn/scikit-learn/pulls). | ||
| - [ ] If your issue is a usage question or does not potentially require changes to the codebase to be solved, then StackOverflow (using the `[scikit-learn]` tag) or our [mailing list](https://lists.sourceforge.net/lists/listinfo/scikit-learn-general) may be a better place to bring it up. For more information, see ("User Questions")[https://lists.sourceforge.net/lists/listinfo/scikit-learn-general]. | ||
|
|
There was a problem hiding this comment.
Small nitpick: I think it should be ["User Questions"] (https.....). The brackets have been interchanged by mistake I think.
There was a problem hiding this comment.
Wow, that's actually a pretty big error haha. Thanks for the notification :)
|
I think it will be really useful to add a line to the PR template saying that our review process takes a good amount of time and that the author must not be discouraged by a lack in activity/review. I've seen quite a few authors feeling demotivated by that. It should also say why that happens (Any addition must be well worth it and should not break anything... (this may be taken care by the inclusion FAQ) The no of full time reviewers are less... Most of the core devs are doing this on their own time etc...) Or maybe add the reason as another FAQ on "Why is my PR not getting any attention?" and link it here. But in any case, the things I feel every contributor should know - 1. The fact that user contributions are very important, no matter how minor they are. 2. If the review is slow, either their PR needs some tinkering, cleaning, benchmarking, convincing etc or more plausibly the reviewers are busy. Either case it should not demotivate them to the extent of closing the PR or discontinuing their work solely because of this reason. |
|
There's a brief note at the bottom of the PR template motivated by the I think part of the answer is that we care a lot about getting things On 23 February 2016 at 21:03, Raghav R V notifications@github.com wrote:
|
| If you are submitting a bug issue: | ||
| - [ ] Please include your operating system type and version number, as well as your Python, scikit-learn, numpy, and scipy versions. This information can be found by runnning [this code snippet](https://gist.github.com/nelson-liu/108ad4a0571e43ac3d80). | ||
| - [ ] Please ensure all code snippets are formatted in appropriate code boxes. See ["Creating and highlighting code blocks"](https://help.github.com/articles/creating-and-highlighting-code-blocks). | ||
|
|
There was a problem hiding this comment.
Add "Please be specific about what estimators/functions are involved and the shape of the data, as appropriate; you may include code snippets or link to a [gist](https://gist.github.com). If an exception is raised, please provide the traceback."
|
Despite the use of task lists in the github blog post about templates, they aren't functional when editing. More commonly it seems ISSUE_TEMPLATE is being used to provide headings where submitters fill in the blanks. I'm happy to keep it as bulleted lists, but I think we can ditch the |
|
@jnothman should we ditch the |
|
Perhaps. They're not checklists for the progress of the PR. Then again we On 24 February 2016 at 15:11, Nelson Liu notifications@github.com wrote:
|
|
Maybe we should mention that these guidelines should be deleted before adding the actual PR/issue description? |
|
Otherwise seems great to me. Thanks! |
| @@ -0,0 +1,13 @@ | |||
| Please try to adhere to the guidelines below as much as possible when submitting your issue. | |||
| - Verify that your issue is not being currently addressed by other [open issues](https://github.com/scikit-learn/scikit-learn/issues) or [pull requests](https://github.com/scikit-learn/scikit-learn/pulls). | |||
There was a problem hiding this comment.
I reckon we should include closed issues (and possibly closed PRs) and use a link like https://github.com/scikit-learn/scikit-learn/issues?q= so they can search through issues and PRs at once.
|
LGTM. I hope it's not too verbose for users. |
| Please try to adhere to the guidelines below as much as possible when submitting your pull request. | ||
|
|
||
| - Please verify that your code satisfies the [code/documentation quality guidelines](http://scikit-learn.org/stable/developers/contributing.html#coding-guidelines). | ||
| - Please prefix the title of your pull request with `[MRG]` if the contribution is complete and should be subjected to a detailed review. Otherwise: |
There was a problem hiding this comment.
"Otherwise:" can be removed I feel.
There was a problem hiding this comment.
I feel like its good to contrast the two options that users might have at this stage.
6a969f8 to
d63109d
Compare
| [code/documentation quality guidelines](http://scikit-learn.org/stable/developers/contributing.html#coding-guidelines). | ||
| - Please prefix the title of your pull request with `[MRG]` if the | ||
| contribution is complete and should be subjected to a detailed review. | ||
| Otherwise: |
There was a problem hiding this comment.
You probably need to leave a new line before "otherwise". Look at how github renders the markdown here
There was a problem hiding this comment.
good point, I meant to leave it on the same line but its probably clearer on a new line.
d63109d to
4cf6b54
Compare
| - Please prefix the title of your pull request with `[MRG]` if the | ||
| contribution is complete and should be subjected to a detailed review. | ||
|
|
||
| Otherwise: |
There was a problem hiding this comment.
Actually I would remove "Otherwise" as mentioned by somebody else previously. Sorry I didn't read that properly initially.
|
Something that didn't hit me originally: the user creating an issue or PR is going to see this markdown files in write mode, rather than preview mode. That means that links are not clickable, code snippets are not highlighted, etc ... Maybe we should just say at the beginning of the template "Click on the Preview tab or hit Control-Alt-P and read carefully" and then at the end "Now you can click on the Write tab to create your issue/PR and please remember to delete the guidelines". |
|
Alternatively we could have external markdown files and just add a link in the template. It does feel a bit like a more "in your face" CONTRIBUTING.md (with the slight but important difference that we can have different templates for issues and PRs). The issue template could have a few sections to fill, like this one: By the way it is quite neat how HTML comments are used to have some text which is visible when writing the issue but not when viewing the issue. We could use the same trick so that guidelines don't have to be deleted. |
|
@lesteve I like your idea of creating external markdown files and just linking them in the template, as the raw output is a lot harder to work with vs the preview output. Maybe then we could build a skeleton of an issue like ownclouds? |
| be accepted. | ||
| - If you are adding an enhancement, you may wish to provide evidence for | ||
| its benefit with distinguishing examples in the code and benchmarks | ||
| in the PR discussion. |
There was a problem hiding this comment.
I would definitely add somewhere that if you are addressing an issue, the title of the PR should be describing the issue, but the text of the PR should have #issuenumber in it, so that a link is created. We often get titles like "fixing #1234" which I don't find very helpful for browsing the pull requests, and also they don't create a link to the issue.
|
I don't think this needs great finessing, as it will be updated once we see its effect. I think it would be good if @nelson-liu addressed the outstanding comments and we merged the result. Please hold off on further nitpicking (if I may say so with a tinge of hypocrisy)! |
I am hoping it doesn't fit the "great finessing" you are talking about, sorry if it does. I am fine if those comments get addressed in a further PR. Personally I would be in favour of:
|
|
Sorry for the delay on fixes, real life happened :) I'll address these changes tomorrow morning, thanks for your patience and reviews |
4cf6b54 to
6ced6d9
Compare
6ced6d9 to
f0f0aff
Compare
|
I've addressed the outstanding issues, and squashed.
|
|
If there aren't any objections I'll merge this tomorrow and we'll see how it goes. |
|
I guess once this gets merged, we can also add a para for commit tags used in sklearn thus addressing issue #6211. |
haha +1 (with at least the same amount of hypochrisy) |
[MRG+2] DOC: add issue and pull request template
|
Merged #6411.
yipikaye!
Thanks to everybody
|
|
hm this does look a bit overwhelming. In particular, as you start out on the |
|
@amueller yeah, that greatly diminishes the readability by default. It'd be convenient if there was some way to start off in the preview tab by default. Alternatively, we could implement @lesteve's idea of putting things in a separate document and link it in the template, thus reducing its verbosity. |
|
we can also open an issue to track this discussion. Well, there is already the contribution guide that we link to, right? Here we can distinguish between PR and issue, which we couldn't do before, right? |
Yup, also it's a lot more "in your face". You're pretty much forced to acknowledge it is there and read it. It's very easy to pass up the "please review the contributing guidelines" notice at the top of issues or PRs |
|
I think the line wrapping makes it particularly ugly. But indeed, hyperlinks are not so pleasant. |
|
I find this a little bit overwhelming. And it looks more like guidelines than a template. |
|
@mblondel I agree. I spent some time looking at what other projects did for their issue / PR templates, and I found one of each that I particularly like. The PR template is: The issue template is: Any thoughts on modeling our templates off of these examples, and elucidating on topics in a linked contribution guidelines page? |
|
Looks like we should keep the discussion going on the original issue now that this PR has been merged, i.e. #6394. |
Added a first draft issue and PR template as discussed at #6394, please let me know what you think.