pylint-dev / pylint

It's not just a linter that annoys you!
https://pylint.readthedocs.io/en/latest/
GNU General Public License v2.0
5.23k stars 1.11k forks source link

`unnecessary-lambda-assignment` false-positive (?) with pytest's `__tracebackhide__` #6894

Open The-Compiler opened 2 years ago

The-Compiler commented 2 years ago

Bug description

pytest has a feature which lets you set a __tracebackhide__ local to suppress the given frame's source code from its output, normally used for assertion helpers:

import pytest

def checkconfig(x):
    __tracebackhide__ = True
    if not hasattr(x, "config"):
        pytest.fail("not configured: {}".format(x))

def test_something():
    checkconfig(42)

It's also supported to set __tracebackhide__ to a callable instead, to only hide the frame if the given condition applies. In my codebase, I often use this with lambdas. From pytest's documentation, slightly altered:

import operator
import pytest

class ConfigException(Exception):
    pass

def checkconfig(x):
    __tracebackhide__ = lambda e: isinstance(e, ConfigException)
    if not hasattr(x, "config"):
        raise ConfigException("not configured: {}".format(x))

def test_something():
    checkconfig(42)

I think this should be considered suitable usage of lambda because of how __tracebackhide__ is just a normal boolean in its simple form. It seems odd to use def to define it.

Granted, though, this doesn't seem widely used at all. In fact, I might be the only one doing this. If you disagree about this being a good idea, feel free to close this!

cc @jpy-git who introduced this checker in #6004.

Configuration

No response

Command used

pylint --disable=all --enable=unnecessary-lambda-assignment x.py

Pylint output

************* Module x
x.py:10:24: C3001: Lambda expression assigned to a variable. Define a function using the "def" keyword instead. (unnecessary-lambda-assignment)

Expected behavior

No message, if the local being assigned to is named __tracebackhide__.

Pylint version

pylint 2.15.0-dev0
astroid 2.12.0-dev0
Python 3.10.5 (main, Jun  6 2022, 18:49:26) [GCC 12.1.0]

OS / Environment

No response

Additional dependencies

No response

Pierre-Sassoulas commented 2 years ago

Adding discussion label and 2.15 milestone so a decision is taken by then.

jacobtylerwalls commented 2 years ago

I think this is just the inherent limit of a style warning--if you know what you're doing and like the style, the check isn't very useful to you. (I disabled this check in music21 when it came out, since all the lambdas it identified were readable and not worth refactoring.)

DanielNoord commented 2 years ago

In an effort to avoid many of my more pressing deadlines I have been working on a little something over on https://github.com/DanielNoord/pylint-pytest-plugin today.

It's a plugin for pylint which in theory should add compatibility for pytest. Instead of traditional plugins, this plugin hacks into the add_message method of pylint which allows it to stop pylint from emitting some of the messages of its "stdlib". Normally plugins are limited to only emitting new messages.

For example, currently this is one of the functional tests: https://github.com/DanielNoord/pylint-pytest-plugin/blob/main/tests/functional/t/tracebackhide_inner_scope.py

The plugin checks whether the message occurs in 1) a function starting with test, 2) a class starting with test or 3) a module that imports pytest in some form (see other tests in that directory).

I'm not sure when I'll finish this plugin and I don't think it's current state is ready for publication, but I do think that this is better handled by a plugin than within pylint itself as @jacobtylerwalls argued above. Let's keep this issue open until a 0.1.0 version of pylint-pytest-plugin is available.

DanielNoord commented 2 years ago

Now available as https://pypi.org/project/pylint-pytest-plugin/.

v0.1.0a1 adds support for __tracebackhide__ as well as recognising (some) fixture definitions. Please see if this fixes your issues!

The-Compiler commented 2 years ago

Probably makes a lot of sense to have this in a pytest-specific plugin! Note however that there is already pylint-pytest with exactly the same aim. However, it might be dead: https://github.com/reverbc/pylint-pytest/issues/31

But perhaps consider forking that and/or taking some inspiration from there? FWIW in qutebrowser, I currently run pylint separately from tests and disable some additional messages there. Not quite sure why I never tried that plugin, I think I just never got around to it and then forgot it again.

DanielNoord commented 2 years ago

Yeah the plugin seems to be dead. Either way I wanted to see how difficult or easy it is to create a pylint plugin. Which has given me some ideas for improving that workflow.

I'll wait for a response in that issue to continue development though.