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

[SIM115] Allow open followed by close #7916

Merged
merged 4 commits into from
Oct 11, 2023

Conversation

harupy
Copy link
Contributor

@harupy harupy commented Oct 11, 2023

Summary

#7907

Test Plan

New test cases

@github-actions
Copy link
Contributor

github-actions bot commented Oct 11, 2023

PR Check Results

Ecosystem

ℹ️ ecosystem check detected changes. (+0, -9, 0 error(s))

airflow (+0, -9)

- airflow/utils/log/file_processor_handler.py:150:13: SIM115 Use context handler for opening files
- airflow/utils/log/file_task_handler.py:482:13: SIM115 Use context handler for opening files
- tests/core/test_logging_config.py:146:17: SIM115 Use context handler for opening files
- tests/core/test_logging_config.py:148:13: SIM115 Use context handler for opening files
- tests/providers/apache/beam/operators/test_beam.py:433:13: SIM115 Use context handler for opening files
- tests/providers/apache/beam/operators/test_beam.py:598:13: SIM115 Use context handler for opening files
- tests/sensors/test_filesystem.py:103:13: SIM115 Use context handler for opening files
- tests/sensors/test_filesystem.py:162:17: SIM115 Use context handler for opening files
- tests/sensors/test_filesystem.py:181:13: SIM115 Use context handler for opening files

Rules changed: 1
Rule Changes Additions Removals
SIM115 9 0 9

@@ -42,3 +42,7 @@
with contextlib.ExitStack() as exit_stack:
exit_stack_ = exit_stack
f = exit_stack_.enter_context(open("filename"))

# OK
open("filename").close()
Copy link
Member

Choose a reason for hiding this comment

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

Could you add a comment why this combination is needed?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Suggested change
open("filename").close()
# OK (file is closed immdeidately)
open("filename").close()

comment like this?

Copy link
Member

Choose a reason for hiding this comment

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

More like "Used to clear an existing file (https://stackoverflow.com/a/2769090/3549270)"

There's Path.touch now but i'm surprised there's nothing similar from clearing a file.

@konstin
Copy link
Member

konstin commented Oct 11, 2023

For append instead of write cases, e.g. https://github.com/apache/airflow/blob/e239dcf644b6e68896923d84dd49e9cbb6271373/airflow/utils/log/file_processor_handler.py#L150, i'd recommend using Path.touch instead, but this might better be a separate rule.

@charliermarsh charliermarsh enabled auto-merge (squash) October 11, 2023 13:46
@charliermarsh charliermarsh added the bug Something isn't working label Oct 11, 2023
@charliermarsh charliermarsh merged commit f670f9b into astral-sh:main Oct 11, 2023
15 checks passed
@harupy harupy deleted the SIM115-close branch October 11, 2023 13:55
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug Something isn't working
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants