This has the potential to simplify and speed up our code checks massively. It looks very mature, has an active community and is used by a whole bunch of major and well-known open-source projects.
We discussed it briefly with Maurits at the Alpine City Sprint, and looking at the very long list of checks, we thought that it might be really good for add-ons and internal add-ons, but maybe a bit too much for core, at least at the beginning, as it might rewrite way too much code
Good thing is that as we are using pre-commit already, switching black/isort/flake8 to ruff would not be too much problem in terms of configuration changes
Well, the ALL checks is not recommended anyway. You can switch on check by check. Also, initially I would not rewrite everything, but start with the checks we have and then extend and fix step by step.
I just ran it on Products.CMFPlone. Insanely fast!
I don't like all that it does though.
$ pipx run ruff check Products/CMFPlone/browser/admin.py
Products/CMFPlone/browser/admin.py:355:45: F401 [*] `plone.volto.browser.migrate_to_volto` imported but unused
Found 1 error.
[*] 1 potentially fixable with the --fix option.
$ pipx run ruff check --fix Products/CMFPlone/browser/admin.py
Found 1 error (1 fixed, 0 remaining).
$ git diff
diff --git a/Products/CMFPlone/browser/admin.py b/Products/CMFPlone/browser/admin.py
index 98d4a3256..32c8367fa 100644
--- a/Products/CMFPlone/browser/admin.py
+++ b/Products/CMFPlone/browser/admin.py
@@ -352,7 +352,7 @@ class Upgrade(BrowserView):
if pm.getInstanceVersion() < "6005":
return False
try:
- from plone.volto.browser import migrate_to_volto
+ pass
except ImportError:
return False
installer = get_installer(self.context, self.request)
It does not realise that the original code can raise an ImportError and the new code never does.
A different one:
$ pipx run ruff check Products/CMFPlone/browser/navtree.py
Products/CMFPlone/browser/navtree.py:156:9: F841 [*] Local variable `layout_view` is assigned to but never used
Found 1 error.
[*] 1 potentially fixable with the --fix option.
$ pipx run ruff check --fix Products/CMFPlone/browser/navtree.py
Found 1 error (1 fixed, 0 remaining).
$ git diff
diff --git a/Products/CMFPlone/browser/navtree.py b/Products/CMFPlone/browser/navtree.py
index bba401a21..605d0ac93 100644
--- a/Products/CMFPlone/browser/navtree.py
+++ b/Products/CMFPlone/browser/navtree.py
@@ -153,7 +153,7 @@ class SitemapNavtreeStrategy(NavtreeStrategyBase):
(portalType is None or portalType not in self.parentTypesNQ):
showChildren = True
- layout_view = getMultiAdapter((context, request), name='plone_layout')
+ getMultiAdapter((context, request), name='plone_layout')
newNode['Title'] = utils.pretty_title_or_id(context, item)
newNode['id'] = item.getId
So it sees that a variable is defined by some calculation but is never used. Its automatic fix is to remove the variable definition, but still do the calculation. Now it is much harder for a human to notice that something is wrong: the line is still executed but has no effect. The right solution would be for a human to check if there is some intended side effect to the calculation, and if not: remove the line. In this case the line can indeed be removed.
So: I am not convinced yet. But maybe we can pass options to not run some fixers. And maybe for pure linting (not fixing) it is fine.
I also would not let it auto-fix the code. And never without detailed review.
First, I would enable checks step by step, supervised.
Second I like the feature to add "noqa: xxx" messages to the code, where it does not comply. Then we are able to grep for it and solve it step by step.
Third, I think ruff is still under heavy development and will evolves over time. We may want to start using some basic features and then slowly stat using it for more.
