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

Fix false-negative of unused-variable on catched exceptions #4519

Closed
wants to merge 2 commits into from

Conversation

ksaketou
Copy link
Contributor

Steps

  • Add yourself to CONTRIBUTORS if you are a new contributor.
  • Add a ChangeLog entry describing what your PR does.
  • If it's a new feature or an important bug fix, add a What's New entry in
    doc/whatsnew/<current release.rst>.
  • Write a good description on what the PR does.

Description

This pull request fixes a false-negative of the unused-variable error message caused in case of catched exception inside another except block. It adds a new function for checking the content of the except block for raise and print statements and also other try/except blocks.

Type of Changes

Type
🐛 Bug fix

Related Issue

Closes #4391

This commit fixes a bug of false-negative on the unused-variable error in case of catched exception when
used in the inner context.
@coveralls
Copy link

Coverage Status

Coverage increased (+0.008%) to 91.854% when pulling cb7fe74 on ksaketou:unused-var into 6d449f9 on PyCQA:master.

@Pierre-Sassoulas
Copy link
Member

Hi @ksaketou, thank you for the merge request. I had a look and the tests are very nice. I feel that the fix although it works, is too specific to the problem and I feel the performance and clarity of the code would be impacted. I don't know what would be the best way to fix this (It's possible to check how flake8 does it because it detect this error), but probably by doing something more generic like checking that the variable was not redefined before being used in all contexts, and not only when there is two imbricated try except. We could merge the test alone and add a TODO where pylint do not work properly yet so we can reuse them later if you want.

@ksaketou
Copy link
Contributor Author

I agree that the code is more specific than it should be, but I think that this case is pretty specific as well so I don't know how I could implement it in a more general way. I thought that since the bug only existed in the try/except cases, that that's what I had to work on. I will check flake8 and try to fix the bug. So, I shouldn't focus only on the try/except cases? Any suggestions are welcome 😄

Of course we can merge just the tests for now until I figure this out :)

@ksaketou
Copy link
Contributor Author

ksaketou commented Jun 1, 2021

I'm working on a new branch for the addition of the tests but when I run python -m pytest, I see tests for other error messages like logging-not-lazy, invalid-length-returned etc failing. Maybe some changes happened on these messages?

@Pierre-Sassoulas
Copy link
Member

Can you try to upgrade your dependencies in the virtualenv ? I think you need to have astroid 2.5.7 now.

@Pierre-Sassoulas Pierre-Sassoulas added Waiting on author Indicate that maintainers are waiting for a message of the author Work in progress labels Jul 29, 2021
@Pierre-Sassoulas
Copy link
Member

Hey @ksaketou there was a nice comment on this topic here. I seriously underestimated the difficulty of the task at hand, this is clearly not a friendly issue for new contributor, so I apologize for that. Knowing that do you think we should close this merge request ?

@Pierre-Sassoulas Pierre-Sassoulas added this to the 2.10.0 milestone Aug 4, 2021
@ksaketou
Copy link
Contributor Author

ksaketou commented Aug 5, 2021

Hello @Pierre-Sassoulas , it's okay, indeed I had difficulty finding a solution that works and is also elegant. I think we can close this PR.

@Pierre-Sassoulas
Copy link
Member

Understood. Sorry again for the bad experience.

@Pierre-Sassoulas Pierre-Sassoulas removed Work in progress Waiting on author Indicate that maintainers are waiting for a message of the author labels Aug 5, 2021
@ksaketou ksaketou deleted the unused-var branch September 2, 2021 17:07
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

False negative unused-variable for catched exception when used in the inner context
3 participants