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

refactor: Use context managers when opening files #1093

Merged
merged 2 commits into from
Feb 1, 2024

Conversation

kesara
Copy link
Member

@kesara kesara commented Jan 31, 2024

No description provided.

Copy link
Member

@jennifer-richards jennifer-richards left a comment

Choose a reason for hiding this comment

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

lgtm

Off topic for the PR, but a couple of those might benefit from slight reorganization - instead of

  with open(...) as file:
    s = do_work()
    file.write(s)

you could refactor as

  s = do_work()
  with open(...) as file:
    file.write(s)

and that will have the benefit that a failure in do_work() won't leave a file around. That'd make sense if such a failure is more likely than a problem opening the file.

Actually, there's also pathlib.Path(...).write_text(...) or write_bytes(...) that will do the latter without the with noise.

@kesara
Copy link
Member Author

kesara commented Feb 1, 2024

lgtm

Off topic for the PR, but a couple of those might benefit from slight reorganization - instead of

  with open(...) as file:
    s = do_work()
    file.write(s)

you could refactor as

  s = do_work()
  with open(...) as file:
    file.write(s)

and that will have the benefit that a failure in do_work() won't leave a file around. That'd make sense if such a failure is more likely than a problem opening the file.

That's where context managers come in handy eh?
It will guarantee that at the exit file will be closed regardless of failure happens during the execution of the do_work() or not.

Actually, there's also pathlib.Path(...).write_text(...) or write_bytes(...) that will do the latter without the with noise.

@jennifer-richards
Copy link
Member

It will guarantee that at the exit file will be closed regardless of failure happens during the execution of the do_work() or not.

Right, the file handle will be closed, but it'll leave behind an empty file. Whether you care about that is a separate question, I'm just pointing out that you could avoid creating it until you know that you have data to put in it.

@kesara
Copy link
Member Author

kesara commented Feb 1, 2024

It will guarantee that at the exit file will be closed regardless of failure happens during the execution of the do_work() or not.

Right, the file handle will be closed, but it'll leave behind an empty file. Whether you care about that is a separate question, I'm just pointing out that you could avoid creating it until you know that you have data to put in it.

Good point. Yeah, this could be better.

@kesara kesara merged commit 534b366 into ietf-tools:main Feb 1, 2024
13 checks passed
@kesara kesara deleted the fix/use-context-managers branch February 1, 2024 18:54
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.

2 participants