All tests are passing except the codecov (not enough test coverage). In the test cov report these lines (line 73-77) are not covered. Maybe because assignment of the variable is only done on line 73.

This line isn’t covered by the tests: I added the try-except block to make sure it fails silently if the file happens to still be open. I’m not sure if this is really needed (and if so, whether the exception block should be tested). Thanks in advance!

Unrelated comment: it's much much preferable to capture a specific Exception rather than the base class. So this code should capture the specific one that is trigger by the problem.

Also, I would always add a small comment on why we capture the exception.

I think we should catch the exception as a PermissionError, and the exception should be tested. However, this should be a Windows specific error, so the test should be skipped when the platform isn't Windows with something like

import sys
import pytest

@pytest.mark.skipif(sys.platform != 'win32', reason="Test is Windows-specific")
def test_windows_specific_functionality():
    # test code here

If you don't have a Windows machine to test this one, it will have to be tested on CI which is going to be annoying unfortunately.

I agree with the general rule but here I think catching a broad exception type makes sense: we are performing an accessory background task (cleanup of old files created by other programs), so we don't want to crash the program if it fails. it might fail for other reasons beside the one mentioned here: another thread may have removed it already between the listdir() and the rmtree() calls, we may not have permission if it was created by another user, etc. . In those cases failing the current report generation would not help, so I suggest we emit a warning (so that if they see the warning often the user might want to report it or manually delete the directory if it is large) and proceed with the main task without raising an error

to test on other platforms than windows (and even on windows actually), use unittest.mock and the pytest monkeypatch fixture to mock rmtree so it raises an exception (set side_effect = OSError() on the mock)

I'm fine with @jeromedockes point: we can not worry about being specific and catch a broad exception

Okay then i will just catch a broad exception and write a test for it, I willl work on it today or tomorrow!

A couple of changes: I moved the name condition to the first if, and I unpacked dir into dir_path and dir_time. dir is a python keyword so it should not be used as a variable name.

This is probably going to break the pre-commit checks

maybe we can tweak the wording to make it clearer that each execution of the report checks for old files and removes them? so that wary users don't wonder if we are trying to schedule something or leave some daemon cleanup process laying around

sound good to me. I will change it!

nitpick: the rest of skrub uses the (newer) pathlib module for those kind of filesystem manipulation rather than os, so for consistency it would be great to use it here, especially since the path you get from _get_output_dir is already a pathlib.Path

for example you can get the mtime with stat, the Path object representing the directory has iterdir() and glob() methods, you can join with the / operator or joinpath, etc.

also maybe we could name folders directories instead? since we use the term 'directory' rather than 'folder' for other variables (same for the function name prune_folder)

finally why do you prefer 2 separate loops for finding the directories and then removing them?

totally agree with the variable naming — I’ll change it, and I’ll switch to using the pathlib module as well. As for the two separate loops: I initially used them for debugging purposes so I could easily print all the folders that were selected. But maybe its better to and even more readable to put it all in one loop. I will change it!

i guess the test could be outside the try block

I wonder if we should have an even stricter check of the name (eg check with a regex there is a timestamp) just to be extra sure we don't erase something we shouldn't ... but I guess "full_data_op_report_" is already very specific 🤔