Reduce use of tempfile (fix #25)

rec commented 1 week ago

Summary by Sourcery

Reduce the use of temporary files by refining the logic for handling temporary file creation and permissions. Fix test assertions to accurately reflect the number of temporary files created. Add a test to ensure consistent permissions for temporary files.

Bug Fixes:

Fix the logic in test assertions to correctly account for the number of temporary files created during tests.

Enhancements:

Improve the handling of temporary file permissions by ensuring consistent permissions across different temporary file scenarios.

Tests:

Add a new test to verify that temporary files have consistent permissions when created with different configurations.

sourcery-ai[bot] commented 1 week ago

Reviewer's Guide by Sourcery

This pull request reduces the use of temporary files and modifies the behavior of file operations in the 'safer' library. The changes primarily affect the test suite and the initialization of the library's main class.

File-Level Changes

Change	Details	Files
Modify file count assertions in test cases	Update assertions to expect one less file after operations Remove redundant assertions Adjust expected file count differences	`test/test_open.py`
Add new test case for temporary file permissions	Create test_tempfile_perms function Test permissions for different temp_file configurations Assert that permissions are consistent across configurations	`test/test_open.py`
Modify temporary file naming in the main class	Add logic to generate a temporary file name when temp_file is True Use parent directory and original filename to create temp file name	`safer/__init__.py`
Remove explicit chmod operation on temporary files	Remove the else clause that set permissions to 0o100644 Rely on shutil.copymode for setting permissions when target file exists	`safer/__init__.py`

Tips

- Trigger a new Sourcery review by commenting `@sourcery-ai review` on the pull request. - Continue your discussion with Sourcery by replying directly to review comments. - You can change your review settings at any time by accessing your [dashboard](https://app.sourcery.ai): - Enable or disable the Sourcery-generated pull request summary or reviewer's guide; - Change the review language; - You can always [contact us](mailto:support@sourcery.ai) if you have any questions or feedback.

rec commented 1 week ago

The AI schtick wasn't bad and I pushed some consequent changes to clean things up.

Erotemic commented 1 week ago

This looks reasonable to me, but your change on line 660:

        if temp_file is True:
            parent, file = os.path.split(target_file)
            temp_file = os.path.join(parent, f'.{file}.tmp-safer')
        super().__init__(temp_file, delete_failures, parent)

Looks like it will mean that 607 will never be called:

        if temp_file is True:
            fd, temp_file = tempfile.mkstemp(dir=parent)
            os.close(fd)

That might be a reasonable thing to do. Also, this isn't necessarily a problem, but it is a behavior change: by using {file}.tmp-safer, you lose the randomized names that could prevent (or perhaps delay) issues with attempts to write to the same file. It looks like mkstemp is doing some complex magic to ensure names are randomized in a thread safe way. I'm not sure if that's worth using here as well or not. However, I do like that the temporary file now gives a much better indication of what the file being written will be called. That is a feature I was interested in.

rec commented 1 week ago

Ah, sorry, I should have written up the change like I said I was going to, but I got distracted by the AI code review! :-D

Two very perceptive comments, thanks!

Line 607 does still get called, exactly in the case where you have a stream which is not a file or a file handle (for example, a socket connection). EDIT: I just checked it by assert False in that clause and 17 tests fail, exactly those cases.

Indeed, I thought for a while about replicating the magic that mkstemp does! But then I realized the following: if you call safer.open on the same file twice at the same time, you're going to get unpredictable results anyway.

I will emphasize this in the documentation (EDIT: I just did) but I'm just going to have a blanket prohibition on trying to write to the same file twice at the same time with safer, because there's no way to make it work.

In that case, the "fixed" temp file name is perfectly OK as there will never be collisions.

rec / safer