-
-
Notifications
You must be signed in to change notification settings - Fork 25.8k
CLN use new license notice line #29246
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
Conversation
I think that if we want to do this we need automated tools otherwise it will quickly become inconsistent. Such tools already exists as explained in #20813 (comment). Imo we should really consider setting up a linter and a pre-commit hook. |
@jeremiedbb yes, we use that in |
A small question because I could not find quickly in the original discussion: did we discuss the option to centralize everything in the LICENSE file similarly to what I'm just wondering what make it easier in terms of maintenance and being up to date in that regards. |
To me the advantage of having it in every file is to make it easy for third party libs to vendor/reuse parts of scikit-learn because they just have to take the file. |
Good point. |
So should we merge this? This should be the least controversial one I think. |
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.
LGTM as a first PR of a set to fix it.
Towards #20813
This changes the license notice line to what we agreed, I can either add the change for authors in the same PR, or a separate PR.
cc @scikit-learn/core-devs