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

Local variable is assigned but never used (F841) cannot be ignored with dummy-variable-rgx in except block #6391

Closed
phillipuniverse opened this issue Aug 7, 2023 · 6 comments · Fixed by #6492
Labels
needs-info More information is needed from the issue author

Comments

@phillipuniverse
Copy link
Contributor

phillipuniverse commented Aug 7, 2023

Example:

try:
    1 + 1
except Exception as _err:
    print("Problem")

Ruff always triggers F841 on the except Exception as _err line:

F841 [*] Local variable `_` is assigned to but never used

Ruff version 0.0.282

@charliermarsh
Copy link
Member

charliermarsh commented Aug 7, 2023

Interestingly, Flake8 doesn't respect the dummy-variable-rgx here either. What's the use-case? Did this occur in a real example, where you want to assign the exception but not use it?

@charliermarsh charliermarsh added the needs-info More information is needed from the issue author label Aug 7, 2023
@phillipuniverse
Copy link
Contributor Author

phillipuniverse commented Aug 7, 2023

Flake8 doesn't respect the dummy-variable-rgx here either.

Hm, that is interesting.

Originally I had written this in a project:

try:
    1 + 1
except:
    logger.exception("Cannot add numbers :(")

And then I got feedback that it would be nice to be able to capture the caught exception in a variable for easy access in pdb and other debuggers. So I thought it would be simple to rewrite as:

try:
    1 + 1
except Exception as _err:
    logger.exception("Cannot add numbers :(")

and was surprised when as _err got rewritten by the autofixer. What I ended up shipping was this:

try:
    1 + 1
except Exception as err:
    logger.exception("Cannot add numbers :(", exc_info=err)

Maybe there is a slight benefit in this final solution, as logger.exception(msg) is equivalent to logger.error(msg, exc_info=True) which under the covers gets the original exception with sys.exc_info().

So it's an easy workaround and maybe worthwhile to keep the exact same behavior as flake8 in this scenario.

@tylerlaprade
Copy link
Contributor

tylerlaprade commented Aug 7, 2023

it would be nice to be able to capture the caught exception in a variable for easy access in pdb and other debuggers

This is the tail wagging the dog, IMHO. My team complained about RET504 removing the variable they introduced for the sole purpose of debugging. I held firm, as I don't believe we should allow the code to become messier for the sake of debugging practices.

@charliermarsh
Copy link
Member

Yeah, hmm... IMO it's strange to use that as _err pattern, but it's also surprising not to respect the dummy-variable-rgx here.

@phillipuniverse
Copy link
Contributor Author

removing the variable they introduced for the sole purpose of debugging

@tylerlaprade if it makes you feel any better for holding firm, there are other ways to expose that return value.

In Pycharm you can check the 'Show Return Values' option

image

In pdb, you can use __return__, detailed tip from SO

charliermarsh added a commit that referenced this issue Aug 11, 2023
## Summary

This PR respects our unused variable regex when flagging bound
exceptions, so that you no longer get a violation for, e.g.:

```python
def f():
    try:
        pass
    except Exception as _:
        pass
```

This is an odd pattern, but I think it's surprising that the regex
_isn't_ respected here.

Closes #6391

## Test Plan

`cargo test`
durumu pushed a commit to durumu/ruff that referenced this issue Aug 12, 2023
## Summary

This PR respects our unused variable regex when flagging bound
exceptions, so that you no longer get a violation for, e.g.:

```python
def f():
    try:
        pass
    except Exception as _:
        pass
```

This is an odd pattern, but I think it's surprising that the regex
_isn't_ respected here.

Closes astral-sh#6391

## Test Plan

`cargo test`
@houmankamali-Zipline
Copy link

I have a similar use case by for walrus assignment:

    while _has_not_timed_out := (last_try_epoch_sec - start_epoch_sec) < timeout_sec:
        if _reached_retry_interval := (time() - last_try_epoch_sec) >= retry_interval_sec:

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
needs-info More information is needed from the issue author
Projects
None yet
Development

Successfully merging a pull request may close this issue.

4 participants