[ruff] Activate flake8-bugbear and flake8-pyi

[Pierre-Sassoulas]

Follow-up to #11912 . The message raised by flake8bugbear were very often false positives. Maybe not such a good linter for the pytest team after all ? Or maybe it can catch issues that took time to fix in the past, as the codebase is of very high quality because it's old and battle tested ?

[bluetech]

Left some comments, the commits made it really easy to review, thanks.

From what I've seen the lints are nice so I vote to 👍 on bugbear.

[bluetech]

Maybe ignore the rule for the entire file (IIRC you add # ruff: noqa: B015 at the top of the file)

[Pierre-Sassoulas]

bdd38f2

[bluetech]

Ooh this is pretty terrible, I suggest we refactor the code to avoid this hidden global state.

[Pierre-Sassoulas]

This one I didn't change yet, I suppose the intent was to do this once instead of multiple time:

    from twisted.trial.itrial import IReporter
    from zope.interface import classImplements

    classImplements(TestCaseFunction, IReporter)

When launching

@hookimpl(wrapper=True)
def pytest_runtest_protocol(item: Item) -> Generator[None, object, object]:
    if isinstance(item, TestCaseFunction) and "twisted.trial.unittest" in sys.modules:
        ut: Any = sys.modules["twisted.python.failure"]
        Failure__init__ = ut.Failure.__init__
        check_testcase_implements_trial_reporter()

I suppose it's a costly operation. Any ideas how to fix or should I just use a more "politically correct" global state ?

[bluetech]

Yes, let's at least not "hide" the global var in the parameter list, I think it's a bad pattern. Perhaps we will deal with the actual global sometime...

[Pierre-Sassoulas]

So should I fix this in a subsequent merge request or in this one ?

[bluetech]

I prefer not to add noqa for real problems so either fix now (move done to a global var rather than a mutable default arg) or ignore B006 for the project and fix later.

[Pierre-Sassoulas]

I used an existing global state from python (8c85144 / https://docs.python.org/3/library/functions.html#dir) let me know what you think.

>>> import sys
>>> "sys" in sys.modules
True
>>> "version_info" in dir()
False
>>> from sys import version_info
>>> "version_info" in dir()
True

[bluetech]

I might be misunderstand how dir works but are sure "classImplements" not in dir() is not always false at the point it is called?

[Pierre-Sassoulas]

Right..

import sys
def in_function_scope():
    if not "version_info" in dir():
        from sys import version_info

        print(f"Imported version_info")
    else:
        print("version_info already imported")


in_function_scope()
in_function_scope()

=>

Imported version_info
Imported version_info

I'm going to just use a global

[Pierre-Sassoulas]

import sys

classImplements_has_run = False

def in_function_scope():
    global classImplements_has_run
    if not classImplements_has_run:
        from sys import version_info


        print(f"Imported version_info")
        classImplements_has_run = True
    else:
        print("version_info already imported")





in_function_scope()
in_function_scope()

Imported version_info
version_info already imported

[bluetech]

👍