Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

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

Open
The-Compiler opened this issue Jun 8, 2022 · 6 comments
Labels
Discussion 🤔 Lib specific 💅 This affect the code from a particular library Needs decision 🔒 Needs a decision before implemention or rejection

Comments

@The-Compiler
Copy link
Contributor

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

@The-Compiler The-Compiler added the Needs triage 📥 Just created, needs acknowledgment, triage, and proper labelling label Jun 8, 2022
@Pierre-Sassoulas Pierre-Sassoulas added Discussion 🤔 and removed Needs triage 📥 Just created, needs acknowledgment, triage, and proper labelling labels Jun 9, 2022
@Pierre-Sassoulas Pierre-Sassoulas added this to the 2.15.0 milestone Jun 9, 2022
@Pierre-Sassoulas
Copy link
Member

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

@jacobtylerwalls
Copy link
Member

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.)

@Pierre-Sassoulas Pierre-Sassoulas added the Needs decision 🔒 Needs a decision before implemention or rejection label Jun 23, 2022
@DanielNoord
Copy link
Collaborator

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
Copy link
Collaborator

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
Copy link
Contributor Author

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: reverbc/pylint-pytest#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
Copy link
Collaborator

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.

@DanielNoord DanielNoord added the Lib specific 💅 This affect the code from a particular library label Aug 23, 2022
@DanielNoord DanielNoord removed this from the 2.15.0 milestone Aug 23, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Discussion 🤔 Lib specific 💅 This affect the code from a particular library Needs decision 🔒 Needs a decision before implemention or rejection
Projects
None yet
Development

No branches or pull requests

4 participants