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

Detect open() calls without with operator #3413

Closed
EugeneZelenko opened this issue Feb 19, 2020 · 8 comments · Fixed by #4372
Closed

Detect open() calls without with operator #3413

EugeneZelenko opened this issue Feb 19, 2020 · 8 comments · Fixed by #4372
Assignees
Labels
Checkers Related to a checker Enhancement ✨ Improvement to a component Good first issue Friendly and approachable by new contributors Help wanted 🙏 Outside help would be appreciated, good for new contributors

Comments

@EugeneZelenko
Copy link

It'll be great to have check that will catch call to open() made without with operator. Probably check should work for other resource-allocating library calls.

@PCManticore PCManticore added Checkers Related to a checker Good first issue Friendly and approachable by new contributors Enhancement ✨ Improvement to a component labels Feb 27, 2020
@Pierre-Sassoulas Pierre-Sassoulas added the Help wanted 🙏 Outside help would be appreciated, good for new contributors label Mar 2, 2021
@Pierre-Sassoulas Pierre-Sassoulas pinned this issue Apr 13, 2021
@DudeNr33
Copy link
Collaborator

I'd be willing to work on this.
From what I've seen this would be a good addition to the existing RefactoringChecker in the form of a consider-using-with refactoring message.

@Pierre-Sassoulas let me know what you think.

@Pierre-Sassoulas
Copy link
Member

Yes, nice refactoring message name :) ! I assigned you to the issue, thank you !

Pierre-Sassoulas added a commit that referenced this issue Apr 23, 2021
* Implement consider-using-with check
* Fix or disable consider-using-with in codebase
* Fix ticket number in ChangeLog
* Move functional test for ``open()`` into separate testfile and exclude this test from running with PyPy

Co-authored-by: Pierre Sassoulas <pierre.sassoulas@gmail.com>
@Pierre-Sassoulas Pierre-Sassoulas unpinned this issue Apr 23, 2021
@benburk
Copy link

benburk commented May 6, 2021

Thanks for the great addition! Unfortunately I'm not seeing it trigger on my end.

$ cat tmp.py
open('test.txt')


$ pylint tmp.py
************* Module tmp
tmp.py:1:0: C0304: Final newline missing (missing-final-newline)
tmp.py:1:0: C0114: Missing module docstring (missing-module-docstring)

----------------------------------------------------------------------
Your code has been rated at -10.00/10 (previous run: -10.00/10, +0.00)


$ pylint --version
pylint 2.8.2
astroid 2.5.6
Python 3.9.4 (default, Apr  9 2021, 09:32:38) 
[Clang 10.0.0 ]```

@DudeNr33
Copy link
Collaborator

DudeNr33 commented May 7, 2021

That is because you did not assign the return value to anything.
Is this a legitimate use case for you?
I guess just the open call itself not, but you could do something like open("foo.txt", "w").write("hello world"). And in this case it would currently not trigger as well.

@EugeneZelenko
Copy link
Author

But isn't file will not be closed in case of open("foo.txt", "w").write("hello world")'? So it'll be reasonable to extend check to cover such cases too.

@DudeNr33
Copy link
Collaborator

DudeNr33 commented May 7, 2021

Yes, it would remain open.
I agree it would make sense, it just wasn't a use case I had in mind when implementing the check.

@hippo91
Copy link
Contributor

hippo91 commented May 14, 2021

@DudeNr33 should i reopen this issue?

@DudeNr33
Copy link
Collaborator

@hippo91 as this issue is already referenced as "closed" in previous release notes, I'd say no.
I will see what needs to be changed to detect the use case of open() outside an assignment, and if you prefer to have an issue to link the pull request against I can open a new one. Otherwise I'd just submit the PR.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Checkers Related to a checker Enhancement ✨ Improvement to a component Good first issue Friendly and approachable by new contributors Help wanted 🙏 Outside help would be appreciated, good for new contributors
Projects
None yet
Development

Successfully merging a pull request may close this issue.

6 participants