Add ruff config to pyproject.toml for devs who are interested #25147

ksunden · 2023-02-03T19:46:34Z

PR Summary

The change to legend_handler was a numpydoc docstring with an errant
blank line after the Parameters section header.
It was flagged by ruff (but not flake8), but I agree that it should be
changed after reviewing numpydoc standards.
Applying the ruff --fix . removed the blank line, and that was the
only flagged error after converting the flake8 config to ruff config.

While I was looking at the config, I realized that some of the E502
(line length) ignores were out of date since we bumped the line length
to 88, so I removed those ignores from both configs.

I did not change any files to adhere to the new limit, only removed
ignores which failed the 79 limit but pass at 88.

One note is that the flake8 config selects C90 (mccabe complexity) but
does not set a max-complexity value, meaning that the check is
ignored. For ruff, a default complexity (10) is selected, and we have
many methods which fail (our worst is 73 in sankey, but ~180 above 10).
As such I removed it from the ruff config, rather than adding ignores,
etc.

At this time, I'm not enabling ruff on CI because it is missing 9 checks
selected by flake8. Once those checks are implemented (or at least the
more commonly applicable ones, not too worried about "semicolon at end
of line", if I'm honest), may look to swap over because it is faster.

(It already takes only ~1 minute for the linter CI to run most of the
time, but if it it can get down to basically the startup/queue time,
that would be just that little bit faster that you may see it before you
context switch)

All of the missing checks are from pycodestyle, but running pycodestyle
outside of flake8 runs single threaded, so the wall clock time savings
don't quite add up.
Combined with the increased complexity of running more than one tool,
not worth it to implement CI yet.

PR Checklist

Documentation and Tests

[n/a] Has pytest style unit tests (and pytest passes)
[n/a] Documentation is sphinx and numpydoc compliant (the docs should build without error).
[n/a] New plotting related features are documented with examples.

Release Notes

[n/a] New features are marked with a .. versionadded:: directive in the docstring and documented in doc/users/next_whats_new/
[n/a] API changes are marked with a .. versionchanged:: directive in the docstring and documented in doc/api/next_api_changes/
[n/a] Release notes conform with instructions in next_whats_new/README.rst or next_api_changes/README.rst

greglucas

This does pass locally for me, so I think you could add it to the pre-commit configuration file and leave the other tests there until the extra pieces are implemented...

Do we want to add an optional.dependencies section to pyproject.toml? Then we could pip install .[dev] or similar.

greglucas · 2023-02-07T03:49:26Z

pyproject.toml

+    "D100",
+    "D101",
+    "D102",
+    "D103",
+    "D104",
+    "D105",
+    "D106",


I'm a bit confused, these are in ignore, but also select. Do we want to just remove them altogether if we aren't going to use them? (ignore takes precedent)

You are right, but that is how they are in the flake8 config, sooo???

My best guess is that that was originally put that way because those are rules we would like to enforce, but have too many failures to individually enumerate them yet.

I ran flake8-to-ruff (a tool distributed separately on pypi by the ruff devs)
The only rule selection difference I made explicitly was to ignore D90 (mccabe cyclomatic complexity) because that didn't actually do anything on flake8 (the config value was null).

Outside of that the handful of rules that are not currently supported by ruff are added to "external" (There may be cause to add to ignores later as well, didn't look too closely at those because they can't be configured as ignores yet anyway)

It did not respect order of the per file ignores so I re-sorted them because that made it much easier to navigate.

Okay, I tried it out, and selecting "D" was largely simply fine...

Looking back at the flake8 config, it contains some comments which indicate that the select was all rules for the numpy convention. But then ignored are all the rules we break regularly.

I already did configure it to use numpy style docstrings, which automatically disables several of the ignored rules (without having to specify) Thus selecting all "D" rules and only ignoring has the same effect, and cleans up the config quite a bit.

Additionally, the following rules from the ignore list had only a single occurrence, so I may as well fix that occurrence and enforce the rule:

D207: Docstring is under-indented

just a single missing space in the closing """ for one docstring

D403: First word of the first line should be properly capitalized

No occurrences, if no objection to enforcing this going forward, may as well enable

E722: do not use bare except, specify exception instead

single occurence in tests, added a noqa comment (could alternatively add to per file ignore if we wish)

Additionally, selecting all pydocstyle (D) enabled the previously unselected D419: Docstring is empty.

There was a single occurrence of an empty docstring, in scale.py:LinearScale, which has a comment stating that the method is there explicitly to prevent inherited docstring.

noqa comment added, enforcing the rule otherwise

The N error codes (enforcing naming conventions) that were included in the ignore list were also never selected (and selecting "N" was rather noisy even with the ignores).
These have been removed

I think incrementally adding linter checks when appropriate is probably going to be easier, so this sounds fine to me.

(I will state that the speed of ruff made this a lot better to test which rules could be enforced now... doing that with flake8 would have taken much much longer)

pyproject.toml

ksunden · 2023-02-07T05:38:08Z

Do we want to make similar edits to the flake8 config?

(Namely selecting all D, then deselecting rules for numpy style/that we fail often)

Upstream pydocstyle has convention handles much like ruff to automatically deselect the convention's invalid rules, but did not easily find a way to apply that to flake8. (It may be possible, but didn't see it documented)

@greglucas did you want to replace flake8 for precommit already?

I don't see a huge point in running both as a precommit because they largely find the same errors.
I was leaning on the conservative side of not automatically fixing discovered errors in a precommit hook (which hooks like trim trailing whitespace or black do, and I do generally like it on those... just haven't used ruff enough to know if its autofixes are good enough)
So I was going to hold off until the rules were implemented and do a precommit/CI followup potentially.
My reasoning was not clogging up pre commit with the same information twice, and using the more complete tool (for now) once ruff is feature compatible, I have no qualms with swapping it out.

I do see the appeal of running this in pre-commit though because that is a particular area where performance can be noticeable, though admittedly even flake8 isn't too bad there because most of the time its only touching a small handful of files.

I could see the argument for "use the fast thing locally, trust flake8 in CI as a backdrop"

ksunden · 2023-02-07T05:44:04Z

lib/matplotlib/tests/test_font_manager.py

@@ -317,7 +317,7 @@ def test_get_font_names():
            font = ft2font.FT2Font(path)
            prop = ttfFontProperty(font)
            ttf_fonts.append(prop.name)
-        except:
+        except:  # noqa: E722


in the alternative, could be more specific about the exception or do except Exception: (skipping KeyboardInterrupt and SystemExit)

I did not dig into which exception(s) would be thrown here.

I think it would be good to make the correct fix here if possible since that is what the purpose of adding these linter checks is :) Hopefully, it wouldn't be too hard to get the exception thrown your way. Or is this a case of it only fails on some remote systems? Worth a shot to fix properly I think.

I suggest to except Exception: here, because:

we certainly don't want to intercept KeyboardInterrupt and SystemExit

the whole try-except block has the purpose to recover and move on if anything goes wrong for a given font path. We certainly don't want to blow up just because some font has problems. Even if we knew the exceptions, I think we would still have to catch them all.

Updated accordingly

ksunden · 2023-02-07T06:00:29Z

Also, 4/10 of our missing rules from flake8 are apparently implemented in ruff now (though hidden behind a feature flag that is off by default/in the pypi version, so not yet fully usable)

But progress nonetheless

greglucas

I agree with you that there isn't any need to get notified twice about precommit errors, so I agree with you just add one or the other and not both.

I've added ruff to one other project now and I thought the --fix worked quite well, and I think it even recursively checked the rules while making the fixes (i.e. fixing one thing didn't cause an error in another that forced the user to rerun the linter).

greglucas · 2023-02-07T15:40:04Z

lib/matplotlib/tests/test_font_manager.py

@@ -317,7 +317,7 @@ def test_get_font_names():
            font = ft2font.FT2Font(path)
            prop = ttfFontProperty(font)
            ttf_fonts.append(prop.name)
-        except:
+        except:  # noqa: E722


I think it would be good to make the correct fix here if possible since that is what the purpose of adding these linter checks is :) Hopefully, it wouldn't be too hard to get the exception thrown your way. Or is this a case of it only fails on some remote systems? Worth a shot to fix properly I think.

greglucas · 2023-02-07T15:41:55Z

pyproject.toml

+    "D100",
+    "D101",
+    "D102",
+    "D103",
+    "D104",
+    "D105",
+    "D106",


I think incrementally adding linter checks when appropriate is probably going to be easier, so this sounds fine to me.

jklymak · 2023-02-07T17:48:11Z

Putting it in the pre-commit check means we should update the developer guide so folks understand they need to use this. I have found some small differences between the current pre-commit hook and what flake8 tells me, so I think devs should be running these things locally, and if we change to ruff we need to know what to do.

ksunden · 2023-02-07T23:24:36Z

At this stage I still consider flake8 to be the canonical linter, but having a passing ruff config gives options for better feedback/editor integrations that are more performant.

Once ruff matches our flake8 functionality I would reconsider (not too far off, at the rate they're going). At that time I'd update ci, pre commit, dev requirements, and dev docs, etc.

I started from exactly matching flake8 (aside from unimplemented rules) but did expand the checked rules slightly, though only with newly enforced rules having 1 (or 0) instances in the library, so not too worried about switching costs from those. (Could also get flake8 checking those now if we want)

jklymak · 2023-02-07T23:26:28Z

I'm not deeply versed in keeping a code base tidy, but I think keeping them consistent during the transition makes sense.

ksunden · 2023-02-08T01:03:15Z

Backported config cleanup to flake8 for consistency while also removing confusing "select and exclude the same code precisely".

D403 (capitalization of first letter of docstring) re-ignored because flake8 flagged things ruff did not (it had been ignored, so no ultimate change)
flake8 flagged two additional bare excepts (E722). ruff ignores if the exception is re-raised (which is honestly pretty reasonable). Added noqa comments

There will be error codes that need to be ignored in ruff once implemented (but it is an error to configure them preemptively)
The two configs should be as equivalent as we can make it now, I believe.

The change to legend_handler was a numpydoc docstring with an errant black line after the Parameters section header. It was flagged by ruff (but not flake8), but I agree that it should be changed after reviewing numpydoc standards. Applying the `ruff --fix .` removed the blank line, and that was the only flagged error after converting the flake8 config to ruff config. While I was looking at the config, I realized that some of the E502 (line length) ignores were out of date since we bumped the line length to 88, so I removed those ignores from both configs. I did not change any files to adhere to the new limit, only removed ignores which failed the 79 limit but pass at 88. One note is that the flake8 config selects `C90` (mccabe complexity) but does not set a `max-complexity` value, meaning that the check is ignored. For ruff, a default complexity (10) is selected, and we have many methods which fail (our worst is 73 in sankey, but ~180 above 10). As such I removed it from the ruff config, rather than adding ignores, etc. At this time, I'm not enabling ruff on CI because it is missing 9 checks selected by flake8. Once those checks are implemented (or at least the more commonly applicable ones, not too worried about "semicolon at end of line", if I'm honest), may look to swap over because it is faster. (It already takes only ~1 minute for the linter CI to run most of the time, but if it it can get down to basically the startup/queue time, that would be just that little bit faster that you may see it before you context switch) All of the missing checks are from pycodestyle, but running pycodestyle outside of flake8 runs single threaded, so the wall clock time savings don't add up. Combined with the increased complexity of running more than one tool, not worth it to implement CI yet.

I already did configure it to use numpy style docstrings, which automatically disables several of the ignored rules (without having to specify) Thus selecting all "D" rules and only ignoring has the same effect, and cleans up the config quite a bit. Additionally, the following rules from the ignore list had only a single occurrence, so I may as well fix that occurrence and enforce the rule: - D207: Docstring is under-indented - just a single missing space in the closing `"""` for one docstring - D403: First word of the first line should be properly capitalized - No occurrences, if no objection to enforcing this going forward, may as well enable - E722: do not use bare except, specify exception instead - single occurence in tests, added a noqa comment (could alternatively add to per file ignore if we wish) Additionally, selecting all pydocstyle (`D`) enabled the previously unselected D419: Docstring is empty. There was a single occurrence of an empty docstring, in scale.py:LinearScale, which has a comment stating that the method is there explicitly to prevent inherited docstring. - noqa comment added, enforcing the rule otherwise The N error codes (enforcing naming conventions) that were included in the ignore list were also never selected (and selecting "N" was rather noisy even with the ignores). These have been removed

D403 re-ignored because flake8 flagged things ruff did not flake8 flagged two additional bare excepts (E722). ruff ignores if the exception is re-raised.

lib/matplotlib/tests/test_backend_qt.py

lib/matplotlib/tests/test_font_manager.py

This got accidentally ignored in matplotlib#25147. We want D213 (matplotlib#17125, matplotlib#17193) https://matplotlib.org/devdocs/devel/document.html#quote-positions

greglucas reviewed Feb 7, 2023

View reviewed changes

ksunden commented Feb 7, 2023

View reviewed changes

greglucas approved these changes Feb 7, 2023

View reviewed changes

github-actions bot added the status: needs rebase label Feb 9, 2023

ksunden added 4 commits February 9, 2023 12:38

backport config changes to flake8

cff05d9

D403 re-ignored because flake8 flagged things ruff did not flake8 flagged two additional bare excepts (E722). ruff ignores if the exception is re-raised.

Use except Exception in place of bare except

f19b57c

ksunden force-pushed the ruff_config branch from f381673 to f19b57c Compare February 9, 2023 18:39

github-actions bot removed the status: needs rebase label Feb 9, 2023

tacaswell added this to the v3.8.0 milestone Feb 9, 2023

tacaswell reviewed Feb 9, 2023

View reviewed changes

lib/matplotlib/tests/test_backend_qt.py Show resolved Hide resolved

tacaswell reviewed Feb 9, 2023

View reviewed changes

lib/matplotlib/tests/test_backend_qt.py Show resolved Hide resolved

tacaswell reviewed Feb 9, 2023

View reviewed changes

lib/matplotlib/tests/test_font_manager.py Show resolved Hide resolved

jklymak added the status: needs revision label Feb 20, 2023

tacaswell removed the status: needs revision label Feb 22, 2023

tacaswell merged commit f9de8aa into matplotlib:main Feb 22, 2023

tkoyama010 mentioned this pull request Dec 7, 2023

Add ruff linter config to pyproject.toml pyvista/pyvista#5304

Merged

story645 mentioned this pull request Dec 31, 2023

[ENH]: Add way to automatically fix flake8 errors #27588

Closed

timhoffm mentioned this pull request Mar 10, 2024

FIX: Restore D213 in flake8 #27904

Merged

Uh oh!

Add ruff config to pyproject.toml for devs who are interested #25147

Add ruff config to pyproject.toml for devs who are interested #25147

Uh oh!

Conversation

ksunden commented Feb 3, 2023

PR Summary

PR Checklist

Uh oh!

greglucas left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

ksunden commented Feb 7, 2023

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

timhoffm Feb 8, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

ksunden commented Feb 7, 2023

Uh oh!

greglucas left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

jklymak commented Feb 7, 2023

Uh oh!

ksunden commented Feb 7, 2023

Uh oh!

jklymak commented Feb 7, 2023

Uh oh!

ksunden commented Feb 8, 2023

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

timhoffm Feb 8, 2023 •

edited

Loading