MNT replace authors and license with standard text #30511
Conversation
StefanieSenger
added
Quick Review
sklearn/externals/_arff.py
Outdated
| ''' | ||
| """ |
There was a problem hiding this comment.
Since this is vendored from external, I think this file should be kept as it was before. I will remove it from this PR, once I have figured out how to circumvent pre-commit for this purpose.
Edit: Done.
doc/sphinxext/sphinx_issues.py
Outdated
| OUT OF OR IN CONNECTION WITH THE SOFTWARE OR THE USE OR OTHER DEALINGS IN | ||
| THE SOFTWARE. | ||
| """ | ||
| """A Sphinx extension for linking to your project's issue tracker.""" |
There was a problem hiding this comment.
This part was probably copied and pasted from somewhere else (vendored since we talked about it the other day). I guess it should be left as is.
doc/sphinxext/doi_role.py
Outdated
| """ | ||
|
|
||
| # Authors: The scikit-learn developers |
There was a problem hiding this comment.
This part was probably copied and pasted from somewhere else (vendored since we talked about it the other day). I guess it should be left as is.
| @@ -3,15 +3,15 @@ | |||
| Caching nearest neighbors | |||
| ========================= | |||
|
|
|||
| This examples demonstrates how to precompute the k nearest neighbors before | |||
| This example demonstrates how to precompute the k nearest neighbors before | |||
There was a problem hiding this comment.
In general, even for small stuff, I would suggest top open a separate PR for unrelated clean-up, there is always the chance to generate side-discussions ...
Leave it like this for this PR, but as a reviewer, this doesn't put me in a good mood when someone decides "it is OK to do just this unrelated change while I am at it" (as if reviewing a PR on a single topic wasn't hard enough 😅)
sklearn/utils/_pprint.py
Outdated
| @@ -4,62 +4,6 @@ | |||
| # Authors: The scikit-learn developers | |||
| # SPDX-License-Identifier: BSD-3-Clause | |||
|
|
|||
| # Copyright (c) 2001, 2002, 2003, 2004, 2005, 2006, 2007, 2008, 2009, 2010, | |||
There was a problem hiding this comment.
I don't know where this comes from but I guess it's kind of the same, we copied and pasted it from another place and we should keep the original licence?
There was a problem hiding this comment.
I will remove it from this PR. But then, one of the previous merges added that the scikit-learn developers were the authors ...
sklearn/utils/src/MurmurHash3.cpp
Outdated
| @@ -1,6 +1,6 @@ | |||
| //----------------------------------------------------------------------------- | |||
| // MurmurHash3 was written by Austin Appleby, and is placed in the public | |||
There was a problem hiding this comment.
I guess that's fine even if we took it from somewhere else. I don't know ...
sklearn/utils/src/MurmurHash3.h
Outdated
| @@ -1,6 +1,6 @@ | |||
| //----------------------------------------------------------------------------- | |||
| // MurmurHash3 was written by Austin Appleby, and is placed in the public | |||
| // domain. The author hereby disclaims copyright to this source code. | |||
There was a problem hiding this comment.
I guess that's fine even if we took it from somewhere else. I don't know ...
|
Thank you, @lesteve. I have removed the vendored files. |
| @@ -1,11 +1,13 @@ | |||
| """This module contains routines and data structures to: | |||
|
|
|||
| # Authors: The scikit-learn developers | |||
There was a problem hiding this comment.
Looking at other places, it seems the authors+licence should not be inside the docstring but after the docstring?
There was a problem hiding this comment.
Oh yes, that's true.
There was a problem hiding this comment.
LGTM with the last comment tackled.
Here is my current thinking:
- if the authors are scikit-learn contributors we use the standard licence (most of the cases)
- if we took the file from somewhere else with little changes we keep the original licence
- if we took the file from somewhere else and then did significant modifications, keeping the original licence plus adding our standard authors + licence is OK I guess ...
That makes absolutely sense to me. I have made the other change. Thanks for going through this. :) |
|
I have enabled auto-merge, I think it is already an improvement and if someone disagrees with some of the choices or approach, further PRs are always an option 😉 |
Reference Issues/PRs
Towards #20813
What does this implement/fix? Explain your changes.
Some variations were not hit by the regex in #29250. This PR builds on this.
Other comment
Also corrected a few typos that I had been collecting and didn't know where else to put.