Drop black, isort, flake8, ... and use Ruff?

Development Testing and Automation
1

I just want to drop this here:
Yesterday I stumbled over "Ruff - An extremely fast Python linter, written in Rust." (from a toot in my timeline).

Ruff can be used to replace Flake8 (plus dozens of plugins), isort, pydocstyle, yesqa, eradicate, pyupgrade, and autoflake, all while executing tens or hundreds of times faster than any individual tool.

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.

It looks like we should give it a try!

Any opinions on this?

2 Likes
2

I used ruff in my latest greenfield project on a larger codebase...stable and fast...no need for staying with flake8

3

It has both VS Code extension and PyCharm plugin.

As well as a pre-commit hook.

1 Like
4

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 :sweat_smile:

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 :smile:

5

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.

6

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.

2 Likes
7

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.

8

Are there any new opinions on this topic ?

I am thinking of switching to Ruff (from Black / flake8) ...

9

Whatever toolset is used, its usage must be documented and the make (or tox) commands be consistent across all Plone packages.

I'm in favor of new tools that run faster—and therefore consume less electricity and reduce carbon emissions—than current tools, provided the amount of work to make the switch is worth the benefits.