Skip to content

Use with open() as: to close file handles#7575

Closed
cclauss wants to merge 1 commit intoArduPilot:masterfrom
cclauss:use-with-open
Closed

Use with open() as: to close file handles#7575
cclauss wants to merge 1 commit intoArduPilot:masterfrom
cclauss:use-with-open

Conversation

@cclauss
Copy link
Copy Markdown
Contributor

@cclauss cclauss commented Mar 25, 2026

Like:

% ruff rule SIM115

open-file-with-context-handler (SIM115)

Derived from the flake8-simplify linter.

What it does

Checks for cases where files are opened (e.g., using the builtin open() function)
without using a context manager.

Why is this bad?

If a file is opened without a context manager, it is not guaranteed that
the file will be closed (e.g., if an exception is raised), which can cause
resource leaks. The rule detects a wide array of IO calls where context managers
could be used, such as open, pathlib.Path(...).open(), tempfile.TemporaryFile()
ortarfile.TarFile(...).gzopen().

Example

file = open("foo.txt")
...
file.close()

Use instead:

with open("foo.txt") as file:
    ...

References

@cclauss cclauss changed the title Use to close file handles Use with open() as: to close file handles Mar 25, 2026
Copy link
Copy Markdown
Contributor

@peterbarker peterbarker left a comment

Choose a reason for hiding this comment

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

I don't have a problem with adding SIM in. But the title on this PR is clear and the patches don't match it.

Please have your PRs do one thing and do it well.

Comment thread build_parameters.py Outdated
Comment thread build_parameters.py Outdated
@cclauss
Copy link
Copy Markdown
Contributor Author

cclauss commented Mar 25, 2026

SIM and flake8 hate each other, so let's focus only on SIM115.

@peterbarker
Copy link
Copy Markdown
Contributor

These patches all look correct.

How were they tested? We can not afford to break this stuff, and if it's wrong it's definitely going to break stuff.

@cclauss
Copy link
Copy Markdown
Contributor Author

cclauss commented Mar 26, 2026

Can you please recommend a test plan?

In all cases, we read or write whole files in a single .read() or .write(), so there is only one command under the context manager and no intervening code to test between the open()/.read()/.close() or open/.write()/.close().

@cclauss
Copy link
Copy Markdown
Contributor Author

cclauss commented Mar 26, 2026

@tpwrules Your review, please.

@peterbarker
Copy link
Copy Markdown
Contributor

Can you please recommend a test plan?

Work out how to run each script so that the changes lines are crossed. Run the script. Make sure the script still does what it is supposed to do.

Incidentally, why are we not changing all of these to use pathlib.Path(...).read_text(encoding=utf8')?!

@cclauss cclauss marked this pull request as draft April 15, 2026 05:48
@cclauss cclauss closed this Apr 25, 2026
@cclauss cclauss deleted the use-with-open branch April 25, 2026 02:22
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants