Use context managers for some file handling in system modules. #65222
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
ansibot
added
affects_2.10
|
The test |
ansibot
added
ci_verified
atombrella
force-pushed
the
solaris_ctx_man
branch
from
840f9a4
to
f5b15c0
Compare
ansibot
added
core_review
atombrella
force-pushed
the
solaris_ctx_man
branch
from
f5b15c0
to
9f6505f
Compare
ansibot
added
community_review
ansibot
added
the
stale_ci
felixfontein
approved these changes
Dec 4, 2019
| f.close() | ||
| except Exception as e: | ||
| self.module.fail_json(msg="failed to read hostname: %s" % | ||
| to_native(e), exception=traceback.format_exc()) |
There was a problem hiding this comment.
This changes behavior slightly (exception is now caught when error happens during closing), but I think that's ok.
| f.close() | ||
| except Exception as e: | ||
| self.module.fail_json(msg="failed to update hostname: %s" % | ||
| to_native(e), exception=traceback.format_exc()) |
| f.close() | ||
| except Exception as e: | ||
| self.module.fail_json(msg="failed to read hostname: %s" % | ||
| to_native(e), exception=traceback.format_exc()) |
| f.close() | ||
| except Exception as e: | ||
| self.module.fail_json(msg="failed to update hostname: %s" % | ||
| to_native(e), exception=traceback.format_exc()) |
ansibot
removed
the
needs_triage
|
@felixfontein Any reason why this has not been merged yet? |
|
TBH I somehow assumed this (or parts of it) was core-support as well. Would have also been nice if anyone else (f.ex. maintainers of the involved modules) would have bothered to take a look as well... |
ansibot
added
needs_rebase
ansibot
added
needs_revision
ansibot
added
collection
ansibot
added
pre_azp
|
Sorry for missing this PR it seems to have slipped through the cracks. We've just come across this PR when going through the old PRs and issues and this one slipped through. We are happy to review a PR against the latest code today to move to context managers, especially since we no longer need old Python support. |
Sign up for free
to subscribe to this conversation on GitHub.
Already have an account?
Sign in.
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.
SUMMARY
Follow-up to #63840 The changes in this PR shouldn't be changing semantics. And it's also much smaller, so hopefully faster to get reviewed and merged.
ISSUE TYPE
COMPONENT NAME
system, core
ADDITIONAL INFORMATION
Context managers are more Pythonic for dealing with resources, like files or connections.