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

PR: Address pylint's "consider-using-with" warnings #20

Merged
merged 2 commits into from
Apr 26, 2021

Conversation

krassowski
Copy link
Contributor

Fixes #19 by adding context managers in all places flagged by consider-using-with. ExitStack was added in places where the simple with statement cannot be used.

@ccordoba12 ccordoba12 added this to the v1.1.0 milestone Apr 25, 2021
@ccordoba12
Copy link
Member

ExitStack was added in places where the simple with statement cannot be used.

I disagree with this change because it makes the code too cumbersome for no good reason. Please skip those Pylint warnigns instead.

@krassowski
Copy link
Contributor Author

Ok, I reverted the ExitStack use as I have no strong opinion here. I agree that the case for having the context manager is not strong here - it may help in some extreme circumstances, but communicate() should normally terminate the process and not terminating the process should not have terrible consequences anyway.

@ccordoba12
Copy link
Member

it may help in some extreme circumstances, but communicate() should normally terminate the process and not terminating the process should not have terrible consequences anyway

Agreed.

Copy link
Member

@ccordoba12 ccordoba12 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks @krassowski!

@ccordoba12 ccordoba12 changed the title Address pylint's "consider-using-with" warnings PR: Address pylint's "consider-using-with" warnings Apr 26, 2021
@ccordoba12 ccordoba12 merged commit b65f4a8 into python-lsp:develop Apr 26, 2021
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.

Linter and tests are failing on due to new "consider-using-with"
2 participants