Skip to content

[SIM115] Allow open followed by close#7916

Merged
charliermarsh merged 4 commits intoastral-sh:mainfrom
harupy:SIM115-close
Oct 11, 2023
Merged

[SIM115] Allow open followed by close#7916
charliermarsh merged 4 commits intoastral-sh:mainfrom
harupy:SIM115-close

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

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
@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