[MRG] DOC Encourage contributors to use keywords to close issue automatically #9954

qinhanmin2014 · 2017-10-19T04:13:29Z

Reference Issue

What does this implement/fix? Explain your changes.

It seems useful to encourage users to use keywords to create link to issue/PR they solve so that these issue/PR can be automatically closed after merging. I also provide a link to the GitHub help page, which seems useful from my perspective.

Any other comments?

I also correct the link in the pull request template.

jnothman · 2017-10-19T04:21:23Z

doc/developers/contributing.rst

@@ -218,10 +218,13 @@ rules before submitting a pull request:
      ``sklearn.utils`` submodule.  A list of utility routines available
      for developers can be found in the :ref:`developers-utils` page.

-    * If your pull request addresses an issue, please use the title to describe


Leave in:

* Give your pull request a helpful title that summarises what your contribution does. In some cases "Fix <ISSUE TITLE>" is enough. "Fix #<ISSUE NUMBER>" is not enough.

jnothman · 2017-10-19T04:26:15Z

doc/developers/contributing.rst

-      the issue and mention the issue number in the pull request description to
-      ensure a link is created to the original issue.
-
+    * If your pull request addresses an issue or continues other's work,


Make this more explicit:

Often pull requests resolve one or more other issues (or pull requests). If merging your pull request means that some other issues or PRs should be closed, you should `use keywords <https://help.github.com/articles/closing-issues-using-keywords/>`_ (e.g., ``Fixes #1234``, ``Closes #1234``; multiple are allowed as long as each is preceded by "Fixes", "Closes" or "Resolves"). Upon merging, those issues will automatically be closed by GitHub.

jnothman · 2017-10-19T04:27:14Z

PULL_REQUEST_TEMPLATE.md

@@ -1,9 +1,9 @@
 <!--
 Thanks for contributing a pull request! Please ensure you have taken a look at
-the contribution guidelines: https://github.com/scikit-learn/scikit-learn/blob/master/CONTRIBUTING.md#Contributing-Pull-Requests
+the contribution guidelines: http://scikit-learn.org/dev/developers/contributing.html#contributing-pull-requests
 -->
 #### Reference Issue


"Reference Issues"

jnothman · 2017-10-19T04:27:50Z

PULL_REQUEST_TEMPLATE.md

 -->
 #### Reference Issue
-<!-- Example: Fixes #1234 -->
+<!-- Example: Fixes #1234 / Closes #1234 -->


How about "Example: Closes #1234. Closes #2345. See also #3456."

qinhanmin2014 · 2017-10-19T05:01:15Z

@jnothman Thanks :) comments addressed along with some minor improvement from myself. See the page from Circle CI.
Personally, could you please consider rushing it into 0.19.1? On the main page, we still link to the stable version of contribution guide.

jnothman · 2017-10-19T07:24:17Z

PULL_REQUEST_TEMPLATE.md

-#### Reference Issue
-<!-- Example: Fixes #1234 -->
+#### Reference Issues/PRs
+<!-- Example: Fixes #1234 / Closes #2345 / See also #3456 -->


I was quite intentional in using . not / in my comment, to suggest that these can be used together. Is there a way that this can be clearer?

@jnothman Sorry for not reading carefully for your comment.
From my perspective, "Example: Closes(Fixes?) #1234. Closes #2345. See also #3456." make me feel a bit confused. For more complex situation, seems that it might be better to mention in the contribution guide.
My original intention here is to add Closes here, since sometimes contributors may not feel that they are fixing something (e.g., continue a PR) and they can use Closes. So, I might use "Example: Fixes #1234 / Closes #2345" here.
Maybe we can add something like "If your PR is simply related to some other issues/PRs, create a link to them without using the keywords (e.g., See also #1234)" in contribution guide, but I'm not sure how useful it will be.
WDYT? I'll follow your final decision. Thanks :)

qinhanmin2014 · 2017-10-19T08:05:35Z

@jnothman I updated according to your suggestion along with some personal improvement. Please have a look. Thanks :)

lesteve

A few comments

lesteve · 2017-10-19T07:47:28Z

PULL_REQUEST_TEMPLATE.md

@@ -1,9 +1,9 @@
 <!--
 Thanks for contributing a pull request! Please ensure you have taken a look at
-the contribution guidelines: https://github.com/scikit-learn/scikit-learn/blob/master/CONTRIBUTING.md#Contributing-Pull-Requests
+the contribution guidelines: http://scikit-learn.org/dev/developers/contributing.html#contributing-pull-requests


I'd rather link to the markdown because it is guaranteed to be up-to-date. Also CONTRIBUTING.md is supposed to be more concise, so it is more likely that people will actually read it (or part of it).

lesteve · 2017-10-19T08:07:00Z

PULL_REQUEST_TEMPLATE.md

-#### Reference Issue
-<!-- Example: Fixes #1234 -->
+#### Reference Issues/PRs
+<!-- Example: Fixes #1234. Closes #2345. See also #3456. -->


I think people are more likely to read what is the template than in the doc. I would add a short sentence mentioning that this is important to automatically close the issue/PR when the PR is merged and maybe add the link to https://github.com/blog/1506-closing-issues-via-pull-requests. Proposal (feel free to reword and improve it):

<! -- Example: Fixes #1234. Closes #2345. See also #3456. #1234 and #2345 will be closed when this PR is merged. A link will be added to #3456. This is important to keep our tracker organised. More details at: https://github.com/blog/1506-closing-issues-via-pull-requests -->

lesteve · 2017-10-19T08:08:51Z

doc/developers/contributing.rst

+    * Often pull requests resolve one or more other issues (or pull requests).
+      If merging your pull request means that some other issues/PRs should
+      be closed, you should `use keywords to create link to them
+      <https://help.github.com/articles/closing-issues-using-keywords/>`_


I like this link better https://github.com/blog/1506-closing-issues-via-pull-requests (which also links to you special keyword link). The reason is your link is more about closing issues via commit message and people may be confused.

qinhanmin2014 · 2017-10-19T08:44:42Z

@lesteve Thanks for the review :)

I'd rather link to the markdown.

I have changed back the link. (note that the original link is not correct because we don't have such a session in .md file.)
Also, what's the relationship between this checklist and this checklist? The statements in both sides are very similar but both sides are missing something which is owned by other side. Should we make them the same?

I think people are more likely to read what is the template than in the doc.

I firmly agree but we might need a balance. It seems that your message is too long and make contributors hard to fill the form.

It's hard for me to reply in the next couple of hours, so if you have some change in mind, you might commit directly.

lesteve · 2017-10-19T09:03:08Z

Also, what's the relationship between this checklist and this checklist? The statements in both sides are very similar but both sides are missing something which is owned by other side.

In my mind, the CONTRIBUTING.md is a subset of the other. This is what is at the top of CONTRIBUTING.md:
Note: This document is a 'getting started' summary for contributing code,
documentation, testing, and filing issues. Visit the Contributing
page for the full contributor's guide.

Note also that a link to CONTRIBUTING.md is shown when opening a PR in github. It may well be that both checklists has diverged slightly. If that's true it should be fixed.

I firmly agree but we might need a balance. It seems that your message is too long and make contributors hard to fill the form.

The goal is to help them to fill it better, feel free to rephrase and/or shorten it. I think I wanted to make it clearer why this was important to use this special syntax.

qinhanmin2014 · 2017-10-19T09:11:15Z

@lesteve Thanks for the reply :)

In my mind, the CONTRIBUTING.md is a subset of the other.

Seems that it's not the fact now. E.g., The last point in the checklist in .md file is not included in the checklist in .rst file. I think we might need a copy paste between the two lists and I have done that for my part. If you disagree, I'll revert.

qinhanmin2014 · 2017-10-19T14:11:12Z

@jnothman @lesteve I think it's again worth a review.
I have opened #9958 to address the inconsistency between checklists in CONTRIBUTING.md and contributing.rst, please have a look. If needed, I can fix that in this PR. Thanks :)

lesteve

Some comments

lesteve · 2017-10-19T14:59:46Z

PULL_REQUEST_TEMPLATE.md

@@ -1,9 +1,15 @@
 <!--
 Thanks for contributing a pull request! Please ensure you have taken a look at
-the contribution guidelines: https://github.com/scikit-learn/scikit-learn/blob/master/CONTRIBUTING.md#Contributing-Pull-Requests
+the contribution guidelines: https://github.com/scikit-learn/scikit-learn/blob/master/CONTRIBUTING.md


You should use this link (goes to the right subsection):
https://github.com/scikit-learn/scikit-learn/blob/master/CONTRIBUTING.md#pull-request-checklist

lesteve · 2017-10-19T15:03:22Z

PULL_REQUEST_TEMPLATE.md

+
+#### Reference Issues/PRs
+<!--
+Example: Fixes #1234. Closes #2345. See also #3456.


Maybe use only only Fix or only Close (here and everywhere else). Fix and Close do the same and the user can look at the link for the list of possible keywords if he wants some variety.

qinhanmin2014 · 2017-10-19T15:15:47Z

@lesteve Thanks, comments addressed.
Maybe I'm still a little bit unsatisfied about the two checklists but if you think it's fine, then I'll also think so :)

lesteve · 2017-10-19T15:31:52Z

CONTRIBUTING.md

+   allowed as long as each one is preceded by a keyword). Upon merging,
+   those issues/PRs will automatically be closed by GitHub. If your pull
+   request is simply related to some other issues/PRs, create a link to
+   them without using the keywords (e.g., ``See also #1234``).


Actually I missed that but you need only single backticks for markdown. Please make sure that there are no other similar copy-and-paste issues.

lesteve · 2017-10-19T15:32:24Z

CONTRIBUTING.md

+   If merging your pull request means that some other issues/PRs should
+   be closed, you should `use keywords to create link to them
+   <https://github.com/blog/1506-closing-issues-via-pull-requests/>`_
+   (e.g., ``Fixes #1234``, ``Closes #1234``; multiple issues/PRs are


When I said "use only Fix or only Close" I meant everywhere. Can you fix it here too and in all the other places this happens. Just use "Fix" or just use "Close" but not both.

qinhanmin2014 · 2017-10-19T15:57:23Z

@lesteve Thanks. Comments addressed.
Sorry that I underestimate the difference between .rst and .md files. This time, I manually check them on Github and Circle.

jnothman · 2017-10-20T06:04:57Z

Thanks again @qinhanmin2014

…ly (scikit-learn#9954)

qinhanmin2014 added 2 commits October 19, 2017 12:05

guide to close issue

90a4ecf

update pull request template

d7880d7

qinhanmin2014 changed the title ~~[MRG] Encourage contributors to use keywords to close issue automatically~~ [MRG] DOC Encourage contributors to use keywords to close issue automatically Oct 19, 2017

jnothman reviewed Oct 19, 2017

View reviewed changes

address comment

3f924f3

jnothman added this to the 0.19.1 milestone Oct 19, 2017

jnothman approved these changes Oct 19, 2017

View reviewed changes

qinhanmin2014 added 2 commits October 19, 2017 16:01

improve

acc1325

typo

f687433

minor improve

c252908

lesteve reviewed Oct 19, 2017

View reviewed changes

update

77f53a4

copy paste of my part (how about the rest?)

e5a9183

personal suggestion

56e122c

TomDLT approved these changes Oct 19, 2017

View reviewed changes

minor improve

02cb052

lesteve reviewed Oct 19, 2017

View reviewed changes

address comment

4a95927

lesteve reviewed Oct 19, 2017

View reviewed changes

qinhanmin2014 added 2 commits October 19, 2017 23:49

address comment

e49dff3

minor fix

a0e52a8

qinhanmin2014 added 2 commits October 20, 2017 00:00

minor improve

b7151fb

Merge remote-tracking branch 'upstream/master' into close-issue-guide

2b43fca

jnothman merged commit 8544ce5 into scikit-learn:master Oct 20, 2017

jnothman pushed a commit to jnothman/scikit-learn that referenced this pull request Oct 20, 2017

DOC Encourage contributors to use keywords to close issue automatical…

ca6b346

…ly (scikit-learn#9954)

qinhanmin2014 deleted the close-issue-guide branch October 20, 2017 07:33

maskani-moh pushed a commit to maskani-moh/scikit-learn that referenced this pull request Nov 15, 2017

DOC Encourage contributors to use keywords to close issue automatical…

8ac2274

…ly (scikit-learn#9954)

jwjohnson314 pushed a commit to jwjohnson314/scikit-learn that referenced this pull request Dec 18, 2017

DOC Encourage contributors to use keywords to close issue automatical…

cf7109e

…ly (scikit-learn#9954)

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[MRG] DOC Encourage contributors to use keywords to close issue automatically #9954

[MRG] DOC Encourage contributors to use keywords to close issue automatically #9954

qinhanmin2014 commented Oct 19, 2017 •

edited

Loading

jnothman Oct 19, 2017

jnothman Oct 19, 2017

jnothman Oct 19, 2017

jnothman Oct 19, 2017

qinhanmin2014 commented Oct 19, 2017 •

edited

Loading

jnothman Oct 19, 2017

qinhanmin2014 Oct 19, 2017

qinhanmin2014 commented Oct 19, 2017

lesteve left a comment

lesteve Oct 19, 2017

lesteve Oct 19, 2017

lesteve Oct 19, 2017

qinhanmin2014 commented Oct 19, 2017 •

edited

Loading

lesteve commented Oct 19, 2017

qinhanmin2014 commented Oct 19, 2017

qinhanmin2014 commented Oct 19, 2017

lesteve left a comment

lesteve Oct 19, 2017

lesteve Oct 19, 2017

qinhanmin2014 commented Oct 19, 2017

lesteve Oct 19, 2017

lesteve Oct 19, 2017

qinhanmin2014 commented Oct 19, 2017

jnothman commented Oct 20, 2017

[MRG] DOC Encourage contributors to use keywords to close issue automatically #9954

[MRG] DOC Encourage contributors to use keywords to close issue automatically #9954

Conversation

qinhanmin2014 commented Oct 19, 2017 • edited Loading

Reference Issue

What does this implement/fix? Explain your changes.

Any other comments?

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

qinhanmin2014 commented Oct 19, 2017 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

qinhanmin2014 commented Oct 19, 2017

lesteve left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

qinhanmin2014 commented Oct 19, 2017 • edited Loading

lesteve commented Oct 19, 2017

qinhanmin2014 commented Oct 19, 2017

qinhanmin2014 commented Oct 19, 2017

lesteve left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

qinhanmin2014 commented Oct 19, 2017

Choose a reason for hiding this comment

Choose a reason for hiding this comment

qinhanmin2014 commented Oct 19, 2017

jnothman commented Oct 20, 2017

qinhanmin2014 commented Oct 19, 2017 •

edited

Loading

qinhanmin2014 commented Oct 19, 2017 •

edited

Loading

qinhanmin2014 commented Oct 19, 2017 •

edited

Loading