-
-
Notifications
You must be signed in to change notification settings - Fork 25.5k
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
[MRG] DOC ask for non-regression test for bug-fix #8850
Conversation
LGTM |
I think in the world of voluntary contributions, presuming something dad
after a week without asking is a bit ... presumptuous
…On 10 May 2017 2:47 am, "Joan Massich" ***@***.***> wrote:
Reference Issue
Add short explanation of regression test do contributing docs. #8679
<#8679>
What does this implement/fix? Explain your changes.
This PR takes over the PR #8879 'cos it seem dead
Any other comments?
------------------------------
You can view, comment on, or merge this pull request online at:
#8850
Commit Summary
- DOC ask for non-regression test for bug-fix
- Apply changes recomended in PR8817
- address all the changes in the previous PR
- Remove tailing spaces
File Changes
- *M* doc/developers/contributing.rst
<https://github.com/scikit-learn/scikit-learn/pull/8850/files#diff-0>
(3)
Patch Links:
- https://github.com/scikit-learn/scikit-learn/pull/8850.patch
- https://github.com/scikit-learn/scikit-learn/pull/8850.diff
—
You are receiving this because you are subscribed to this thread.
Reply to this email directly, view it on GitHub
<#8850>, or mute the
thread
<https://github.com/notifications/unsubscribe-auth/AAEz6xC71wZepSJtrdvFc3oq_35Tl8q7ks5r4Ji0gaJpZM4NVlmK>
.
|
Travis failure is unrelated and should be fixed by re-triggering the build: https://github.com/kennethreitz/requests/issues/4006 |
@jnothman I see. I'll do next time. My apologies to @puneetmathurDS. |
2 comments:
|
@lesteve CONTRIBUTING.md has no section regarding converage. Only a bullet list telling you how to check coverage. However, both CONTRIBUTING.md and contributing.rst have a section regarding BUGFIX, Shall we explain the non-regression test there? BTW, is there anyway to link RST and MD files so that only one needs to be edited? |
Use your best judgement.
Nope, this is a bit unfortunate but there you go. Side-comment: I am not 100% sure about this but because we use squash and merge it is very likely that the commits in master will appear under the name of the first committer. You did not really use anything in the original PR so starting from scratch was probably the best way forward. |
Actually both have a Filing Bug section not a BugFix section. If you file a bug you don't have to supply a non-regression test. So it should not go here. |
I've moved the explanation to somewhere that was making more or less sense and that it was common to both files. Maybe needs some rewriting. |
The best place I could find where to add this piece of information was in the same bullet point as "Documentation and high-coverage tests are necessary for enhancements to be accepted." |
CONTRIBUTING.md
Outdated
to be accepted. | ||
- Documentation and high-coverage tests are necessary for enhancements to be | ||
accepted. Bug-fixes or new features should be provided with non-regression | ||
tests. Tests that verify the correct behavior of the fix or feature. In this |
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.
"Tests that verify" -> These tests verify
doc/developers/contributing.rst
Outdated
@@ -222,6 +222,11 @@ rules before submitting a pull request: | |||
|
|||
* Documentation and high-coverage tests are necessary for enhancements | |||
to be accepted. | |||
Bug-fixes or new features should be provided with non-regression tests. | |||
Tests that verify the correct behavior of the fix or feature. In this |
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.
"Tests that verify" -> These tests verify
By that measure I'm long gone to dust ^^ |
LGTM, maybe link to wikipedia for regression test? |
CONTRIBUTING.md
Outdated
to be accepted. | ||
- Documentation and high-coverage tests are necessary for enhancements to be | ||
accepted. Bug-fixes or new features should be provided with non-regression | ||
tests. Tests that verify the correct behavior of the fix or feature. In this |
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.
I would be even more explicit for bug-fix to say that the test needs to be failing on master and passing in the PR branch.
I think this is good enough, let's not spend more time nitpicking on this one. Merging, thanks a lot! |
Reference Issue
Fixes #8679
What does this implement/fix? Explain your changes.
This PR takes over the PR #8817 'cos it seems dead
Any other comments?