MNT Add author/license note where missing and add the linter #29477

adrinjalali · 2024-07-12T11:25:21Z

This adds a linter and adds the notice where missing.

github-actions · 2024-07-12T11:26:40Z

✔️ Linting Passed

All linting checks passed. Your pull request is in excellent shape! ☀️

_{Generated for commit: 660d1a3. Link to the linter CI: here}

adrinjalali · 2024-07-16T09:42:39Z

@betatim @lorentzenchr , wanna have a look? This should close #20813 maybe?

betatim · 2024-07-17T07:34:20Z

sklearn/ensemble/_hist_gradient_boosting/grower.py

@@ -5,6 +5,9 @@
 the gradients and hessians of the training data.
 """

+# Authors: The scikit-learn developers
+# SPDX-License-Identifier: BSD-3-Clause
+
 # Author: Nicolas Hug


Do we keep lines like this? I don't remember if we discussed/decided something, my knee jerk reaction while looking at this PR is "we should remove it because it is confusing to have authors and author".

My understanding is that we'll remove them all at some point. It can be done in follow-up PRs though

Another PR was supposed to remove these, this was missed.

pyproject.toml

betatim

Thanks for doing all these administrative fix ups.

One question about having "author" and "authors", but otherwise looks good to me!

jeremiedbb · 2024-07-17T09:26:31Z

pyproject.toml

+    # E721 is in preview (july 2024) and gives many false positives.
+    # Use `is` and `is not` for type comparisons, or `isinstance()` for
+    # isinstance checks
+    "E721",
+    # F841 is in preview (july 2024), and we don't care much about it.
+    # Local variable ... is assigned to but never used
+    "F841",


Instead of ignoring them, should we just fix them first ?
Looks like you've already started it in #29501. When it's merged, we can remove this filter, right ?

yes and now, preview enables some stuff in existing enabled rules that we might not want to fix (astral-sh/ruff#12343)

jeremiedbb · 2024-07-17T09:27:53Z

pyproject.toml

+    # Use `is` and `is not` for type comparisons, or `isinstance()` for
+    # isinstance checks
+    "E721",
+    # F841 is in preview (july 2024), and we don't care much about it.


and we don't care much about it.

well, it shouldn't be too hard to fix and I think it hurts readability to have useless variables :)

I'm actually not sure. I'm happy to fix them in a separate PR, but I'm not sure if it helps readability.

Let's keep the filters for now, and see what we do about them afterwards.

The thing is that these preview rules will become stable at some point and I don't think we should add filters for new rules to which we don't comply. I think that we should follow ruff/flake8/black rules as they come, unless it represents a major change that everyone dislike.

jeremiedbb · 2024-07-17T09:29:31Z

.pre-commit-config.yaml

@@ -7,7 +7,7 @@ repos:
    -   id: trailing-whitespace
 -   repo: https://github.com/astral-sh/ruff-pre-commit
    # Ruff version.
-    rev: v0.2.1
+    rev: v0.5.1


Should we bump the min version in _min_dependencies and update the lock files then ?

Otherwise I can imagine that a user has an older version and then the CI complains for errors that he doesn't see locally.

This PR also updates _min_dependencies, should I try to recreate the lock files? I don't mind, but that happens anyway the monday after we merge?

Oh I missed that. For the lock files, it should be fine because it's just the linter, but usually it's better to regenerate in the PR updating the dependencies so that we can see and fix any issue right away.

sklearn/utils/_show_versions.py

jeremiedbb · 2024-07-17T09:34:54Z

pyproject.toml

 ]

+[tool.ruff.lint.flake8-copyright]
+notice-rgx = "\\#\\ Authors:\\ The\\ scikit\\-learn\\ developers\\\n\\#\\ SPDX\\-License\\-Identifier:\\ BSD\\-3\\-Clause"


can we add an extra blank line after because currently there are occurences of

# authors... # copyright import xxx

and

# authors... # copyright... import xxx

I prefer the latter

adrinjalali · 2024-07-17T14:40:09Z

I think this is ready for a merge?

betatim · 2024-07-17T15:17:09Z

I update the lockfiles and pushed them to this PR. Let's see what happens.

I was expecting to see ruff in the list of things changed in the lockfiles, but I can't spot it?

betatim · 2024-07-17T15:47:16Z

The answer to why ruff doesn't appear in the lockfiles is that ruff gets installed in the workflow.

jeremiedbb · 2024-07-17T16:30:06Z

The answer to why ruff doesn't appear in the lockfiles is that ruff gets installed in the workflow.

haha I haven't thought about that :) then definitely no need to regenerate them here.

examples/applications/plot_digits_denoising.py

examples/classification/plot_classifier_comparison.py

examples/cluster/plot_cluster_iris.py

examples/cluster/plot_digits_agglomeration.py

jeremiedbb · 2024-07-17T16:37:16Z

examples/cluster/plot_hdbscan.py

+# Authors: The scikit-learn developers
+# SPDX-License-Identifier: BSD-3-Clause
 # %%


hum the extra blank line doesn't seem to work

betatim · 2024-07-18T15:00:00Z

I undid the lockfile change.

adrinjalali · 2024-07-19T15:39:52Z

Did more cleanups, we haven't even touched the cython files, maybe for another PR?

jeremiedbb · 2024-07-19T16:05:25Z

we haven't even touched the cython files, maybe for another PR?

sure :)

jeremiedbb

Looks like the extra blank line thing is not working, see
#29477 (comment). I saw many occurrences. Otherwise LGTM.

adrinjalali · 2024-07-22T12:00:41Z

@jeremiedbb I don't know how to fix the newline issue in linter, but I added all missing newlines with

$ git grep -n '# SPDX-License-Identifier: BSD-3-Clause' | awk -F: '
{
    filename=$1;
    linenum=$2;
    cmd="sed -n " linenum+1 "p " filename;
    cmd | getline nextline;
    close(cmd);
    if (nextline !~ /^$/) {
        insertCmd="sed -i \"" linenum "a\\\\\n\" " filename;
        system(insertCmd);
    }
}'

(code generated partly by chatgpt)

jeremiedbb · 2024-07-22T15:58:44Z

I don't know how to fix the newline issue in linter

Alright, not a big issue though, thanks for trying :)
Let's merge then

pyproject.toml

Charlie-XIAO · 2024-07-23T14:04:28Z

The linter seems to raise false positives CPY001 if the file uses CRLF. Since the recommended setup of Git is core.autocrlf true (so CRLF on Windows) I imagine most developers using Windows (though maybe none except me 🤣) to experience the issue... Not sure if it's a bug in ruff or there is some kind of setting that can be activated?

adrinjalali · 2024-07-23T16:30:06Z

Would it then make sense to change that git config maybe? Could also change the regular expression to accept CRLF as well.

Charlie-XIAO · 2024-07-23T16:55:44Z

If disabling core.autocrlf on Windows there will tons of diffs because of different EOL IIRC 😢 so a fix might be better. Perhaps changing \n to \r\n??

jeremiedbb · 2024-07-25T12:31:57Z

would modifying the regex with \r?\n work ?

…learn#29477) Co-authored-by: Jérémie du Boisberranger <jeremie@probabl.ai>

adrinjalali added 6 commits June 18, 2024 20:37

MNT enable ruff-copyright-check

f05d7c4

add missing notes

ed17f96

minor fixes

b2d1fc2

CLN fix the remaining ruff issues

6c1d144

merge main

206f421

fix merge conflict

ede3462

adrinjalali changed the title ~~Add author/license note where missing and add the linter~~ MNT Add author/license note where missing and add the linter Jul 12, 2024

adrinjalali added 3 commits July 12, 2024 14:17

make preview explicit selection

6afe100

revert some changes

6a394a3

revert some changes

575e2e0

adrinjalali mentioned this pull request Jul 16, 2024

Enabling specific preview rules astral-sh/ruff#12343

Open

adrinjalali added 2 commits July 16, 2024 11:25

working

b6be6b9

bump ruff version

2691838

adrinjalali marked this pull request as ready for review July 16, 2024 09:29

Merge remote-tracking branch 'upstream/main' into ruff/copyright

b16d1c4

betatim reviewed Jul 17, 2024

View reviewed changes

pyproject.toml Show resolved Hide resolved

betatim approved these changes Jul 17, 2024

View reviewed changes

jeremiedbb reviewed Jul 17, 2024

View reviewed changes

adrinjalali added 3 commits July 17, 2024 16:24

remove leftover authors

b8edc15

Jeremie's comments

96f80c7

Merge remote-tracking branch 'upstream/main' into ruff/copyright

1560f98

jeremiedbb reviewed Jul 17, 2024

View reviewed changes

betatim force-pushed the ruff/copyright branch from ad85626 to 1560f98 Compare July 18, 2024 14:59

adrinjalali added 4 commits July 19, 2024 17:23

Merge remote-tracking branch 'upstream/main' into ruff/copyright

b079b2e

remove more leftover authors

b7bb828

remove Code Source instances

1c0d9e6

remove duplicate license statements

b5db517

jeremiedbb added the No Changelog Needed label Jul 19, 2024

jeremiedbb reviewed Jul 19, 2024

View reviewed changes

adrinjalali added 2 commits July 22, 2024 12:55

Merge remote-tracking branch 'upstream/main' into ruff/copyright

d7861e4

ensure the newline after

3c71f31

Merge branch 'main' into ruff/copyright

074136d

jeremiedbb reviewed Jul 22, 2024

View reviewed changes

pyproject.toml Outdated Show resolved Hide resolved

Update pyproject.toml

660d1a3

jeremiedbb merged commit 3e127a3 into scikit-learn:main Jul 22, 2024
30 checks passed

adrinjalali mentioned this pull request Jul 30, 2024

MNT fix copyright rule to handle \r\n endings #29585

Merged

MarcBresson pushed a commit to MarcBresson/scikit-learn that referenced this pull request Sep 2, 2024

MNT Add author/license note where missing and add the linter (scikit-…

5588b81

…learn#29477) Co-authored-by: Jérémie du Boisberranger <jeremie@probabl.ai>

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

MNT Add author/license note where missing and add the linter #29477

MNT Add author/license note where missing and add the linter #29477

adrinjalali commented Jul 12, 2024 •

edited

Loading

github-actions bot commented Jul 12, 2024 •

edited

Loading

adrinjalali commented Jul 16, 2024

betatim Jul 17, 2024 •

edited

Loading

jeremiedbb Jul 17, 2024

adrinjalali Jul 17, 2024

betatim left a comment

jeremiedbb Jul 17, 2024

adrinjalali Jul 17, 2024

jeremiedbb Jul 17, 2024 •

edited

Loading

adrinjalali Jul 17, 2024

jeremiedbb Jul 17, 2024

jeremiedbb Jul 17, 2024

adrinjalali Jul 17, 2024

jeremiedbb Jul 17, 2024

jeremiedbb Jul 17, 2024

adrinjalali commented Jul 17, 2024

betatim commented Jul 17, 2024 •

edited

Loading

betatim commented Jul 17, 2024

jeremiedbb commented Jul 17, 2024

jeremiedbb Jul 17, 2024

betatim commented Jul 18, 2024

adrinjalali commented Jul 19, 2024

jeremiedbb commented Jul 19, 2024

jeremiedbb left a comment

adrinjalali commented Jul 22, 2024 •

edited

Loading

jeremiedbb commented Jul 22, 2024

Charlie-XIAO commented Jul 23, 2024

adrinjalali commented Jul 23, 2024

Charlie-XIAO commented Jul 23, 2024

jeremiedbb commented Jul 25, 2024

MNT Add author/license note where missing and add the linter #29477

MNT Add author/license note where missing and add the linter #29477

Conversation

adrinjalali commented Jul 12, 2024 • edited Loading

github-actions bot commented Jul 12, 2024 • edited Loading

✔️ Linting Passed

adrinjalali commented Jul 16, 2024

betatim Jul 17, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

betatim left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jeremiedbb Jul 17, 2024 • edited Loading

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

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

adrinjalali commented Jul 17, 2024

betatim commented Jul 17, 2024 • edited Loading

betatim commented Jul 17, 2024

jeremiedbb commented Jul 17, 2024

Choose a reason for hiding this comment

betatim commented Jul 18, 2024

adrinjalali commented Jul 19, 2024

jeremiedbb commented Jul 19, 2024

jeremiedbb left a comment

Choose a reason for hiding this comment

adrinjalali commented Jul 22, 2024 • edited Loading

jeremiedbb commented Jul 22, 2024

Charlie-XIAO commented Jul 23, 2024

adrinjalali commented Jul 23, 2024

Charlie-XIAO commented Jul 23, 2024

jeremiedbb commented Jul 25, 2024

adrinjalali commented Jul 12, 2024 •

edited

Loading

github-actions bot commented Jul 12, 2024 •

edited

Loading

betatim Jul 17, 2024 •

edited

Loading

jeremiedbb Jul 17, 2024 •

edited

Loading

betatim commented Jul 17, 2024 •

edited

Loading

adrinjalali commented Jul 22, 2024 •

edited

Loading