This re-enables the pylint too-many-positional-arguments for the main
application code. It's still disabled for tests because that's how you pull in
pytlint fixtures, and I don't think we want to push people into being creative
with fixtures just to get around that.
When functions are called with many positional arguments the reader has to do
a bit of heavy lifting to figure out in what position a value is being passed,
and it's easier to make mistakes. So I would like to encourage using keyword
arguments for long argument lists.
I've set the `max-positional-arguments` to a completely arbitrary 7, from a
completely arbitrary 5, because there were many more violations under 7. If
like 99% of our functions fit under 7 it's probably fine.
Regarding the exceptions:
* objreg.register: I grepped it and it looks like everything is only passing
the first two args as positional already, lucky!
* `_get_color_percentage`: only one usage of it, but I was in "add directive
comment" mode
* update_3rdparty.py: only one usage, already using kwargs
* pyqtProperty: idk
* commands.py: "its complicated". Many methods in this file map to commands
used in qutebrowser's command mode. In that case it's usual for them to be
called as flags, rather than positional. But it could be complicated to wade
into that, and having one file excluded isn't so bad.
With the upgrade to MarkupSafe 3.0, something funny happened when trying to pass
the GUIProcess object to jinja after launching a userscript:
[...]
File "[...]/qutebrowser/browser/qutescheme.py", line 291, in qute_process
src = jinja.render('process.html', title=f'Process {pid}', proc=proc)
^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
File "[...]/qutebrowser/utils/jinja.py", line 123, in render
return environment.get_template(template).render(**kwargs)
^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
[...]
File "html/process.html", line 11, in block 'content'
File "[...]/lib/python3.11/site-packages/markupsafe/__init__.py", line 42, in escape
if hasattr(s, "__html__"):
^^^^^^^^^^^^^^^^^^^^^^
RuntimeError: wrapped C/C++ object of type GUIProcess has been deleted
This can be reproduced with:
qutebrowser --temp-basedir ':cmd-later 0 spawn -u -o /bin/echo test'
We pass the `GUIProcess` to the Jinja template as `proc`, which then formats it as
`{{ proc }}`` (to stringify it). For some reason, with the newest MarkupSafe/Jinja
versions, this now triggers the `if hasattr(s, "__html__")` check in MarkupSafe
(which has been around for a while). That then presumably causes PyQt to try and
access the underlying C++ object for `GUIProcess``, but that has already been
deleted.
But why is it deleted in the first place, if we keep track of even completed
processes data ever since we added `:process` in a3adba81c? It looks like the Qt
parent-child relationship is the culprit here: When we pass a parent to the
`GUIProcess`` from the userscript runner, it will get deleted as soon as said
runner is cleaned up (which happens after the userscript has finished).
We probably never noticed this before because we only accessed data from the
Python wrapper and not from the C++ side, but it still seems like a good idea
to avoid passing a parent for a long-lived object (with time-based cleanup) in
the first place.
Added in 3.3.0:
https://pylint.pycqa.org/en/latest/whatsnew/3/3.3/index.html
Some of those arguments could probably indeed be keyword-only,
but for some of the functions shown by pylint, those are qutebrowser command
handlers where a positional argument has different semantics.
Did run with ruff pretending to use Python 3.10,
because otherwise it won't reformat those:
ruff check --select 'UP035' --fix --config 'target-version = "py310"' --unsafe-fixes
This is because collections.abc.Callable inside Optional[...] and Union[...] is
broken with Python 3.9.0 and 3.9.1:
https://github.com/asottile/pyupgrade/issues/677https://github.com/astral-sh/ruff/issues/2690https://github.com/python/cpython/issues/87131
However, pylint can detect problematic usages (of which we only have one),
so we might as well use the new thing everywhere possible for consistency.
Also see #7098
Florian Bruhin
Marcel Schilling
toofar
Willow Barraco
qutebrowser bot