-
-
Notifications
You must be signed in to change notification settings - Fork 1.2k
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
Enhancement #3413 consider-using-with
#4372
Merged
Pierre-Sassoulas
merged 13 commits into
pylint-dev:master
from
DudeNr33:enhancement-3414-consider-using-with
Apr 23, 2021
Merged
Enhancement #3413 consider-using-with
#4372
Pierre-Sassoulas
merged 13 commits into
pylint-dev:master
from
DudeNr33:enhancement-3414-consider-using-with
Apr 23, 2021
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
consider-using-with
consider-using-with
Hm, it seems that the ordinary |
Pierre-Sassoulas
approved these changes
Apr 19, 2021
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thank you, great new checker and a lot of work done !
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Steps
doc/whatsnew/<current release.rst>
.Description
This PR implements a new refactoring message
consider-using-with
inside the existingRefactoringChecker
class.This message is triggered if certain constructs may be replaced with a
with
block to ensure the correct release of allocated resources.The following cases are covered (I took this blog post as a reference):
Resource-allocating assignments
io.IOBase
(open()
,codecs.open()
, etc.)subprocess.Popen
multiprocessing.context.BaseContext.Pool
thread.ThreadPoolExecutor
andprocess.ProcessPoolExecutor
fromconcurrent.futures
Methods that are also called when using the object as context manager
threading
module, e.g.threading.Lock.acquire()
BaseManager.start()
andSyncManager.start()
frommultiprocessing.managers
The current implementation works by checking the assignments/calls against predefined sets of callables of the Python standard library.
After adding this new check some files in the codebase were flagged when running Pylint on itself.
These files have been fixed as well, or the message disabled for the offending lines if transforming into a
with
statement was not possible.Trying to dynamically determine if an assignment or call could be replaced by a
with
block seems possible for easy cases (e.g. if__enter__
returns justself
or calls the same method the currently checked call does), but I think that could become either very incomplete (missing more complex cases) or error prone (reporting false positives).However, it could then also check code outside standard library.
Maybe this could be implemented as an optional checker with a separate PR.
Type of Changes
Related Issue
Closes #3413