Skip to content

Add remove_recursive and remove_recursive_absolute to DirAccess#120860

Open
Elip100 wants to merge 2 commits into
godotengine:masterfrom
Elip100:master
Open

Add remove_recursive and remove_recursive_absolute to DirAccess#120860
Elip100 wants to merge 2 commits into
godotengine:masterfrom
Elip100:master

Conversation

@Elip100

@Elip100 Elip100 commented Jul 2, 2026

Copy link
Copy Markdown
Contributor

What problem(s) does this PR solve?

Additional information

This just exposes an existing DirAccess function to allow its use in GDScript. The function allows you to recursively remove the contents of a directory. I don't know if there is an important reason why it wasn't exposed in the first place, but I proposed this idea a while ago, and several people agreed it would be a nice QOL feature, because there is not currently a DirAccess method that allows you to remove a non-empty directory easily. From my testing, it works perfectly fine, and I can't find a reason to not include it.

See #120860 (comment) for new implementation

@Elip100 Elip100 requested review from a team as code owners July 2, 2026 16:09
@AThousandShips

AThousandShips commented Jul 2, 2026

Copy link
Copy Markdown
Member

I'm not sure this should be exposed in itself, it doesn't match the rest of the usage of DirAccess, none of the other mutating methods use the current directory (also what happens after this is called? The directory itself remaining is confusing as well, I don't really see the usefulness in that as you'll need to just step up and delete the parent directory, and if you want to delete a subdirectory you have to step into it and delete it like this

@Elip100

Elip100 commented Jul 2, 2026

Copy link
Copy Markdown
Contributor Author

I'm not sure this should be exposed in itself, it doesn't match the rest of the usage of DirAccess, none of the other mutating methods use the current directory (also what happens after this is called? The directory itself remaining is confusing as well, I don't really see the usefulness in that as you'll need to just step up and delete the parent directory, and if you want to delete a subdirectory you have to step into it and delete it like this

I agree that the current implementation is definitely a bit strange and inconsistent with other DirAccess functions. What if this erase_contents_recursive method was instead changed into remove_recursive and remove_recursive_absolute, which could function more like the original proposed idea? To me that would make a lot more sense in the context of DirAccess. I thought about doing that as a PR instead, but decided I would try just exposing the existing implementation first before trying anything else.

@AThousandShips

Copy link
Copy Markdown
Member

I think that would be much more appropriate, especially to match what the requested feature is

@AThousandShips AThousandShips changed the title Expose DirAccess::erase_contents_recursive() to GDScript Expose DirAccess::erase_contents_recursive() to scripting Jul 2, 2026
@lawnjelly

Copy link
Copy Markdown
Member

I don't know if there is an important reason why it wasn't exposed in the first place

No idea, but it does handily stop users shooting themselves in the foot. 😁

Erasing directories and their contents in one command is a pretty potent weapon (I don't think the OS offers this directly).

@bluebirdsan

bluebirdsan commented Jul 2, 2026

Copy link
Copy Markdown

I agree, that it would be better to have remove_recursive and remove_recursive_absolute.
Only exposing the erase_contents_recursive would still require multiple steps to remove the directory with all it's contents.
I think it should be a one function thing (should work like if you'd select a folder in a file explorer and hit delete - removes the selected folder with all its contents).

Erasing directories and their contents in one command is a pretty potent weapon (I don't think the OS offers this directly).

I think if this is split into two commands (remove contents and then the directory) then it isn't that much different, because if you remove contents first, you already lost everything useful inside, so the one command thing would just be more convenient :D + if anyone would pass a path without checking, if they should remove it, then they're just looking for trouble anyway.

EDIT: I also think that most people would use this function on "user://" paths, which at most would break the game and not the whole system.

@Elip100

Elip100 commented Jul 3, 2026

Copy link
Copy Markdown
Contributor Author

it does handily stop users shooting themselves in the foot. 😁

Erasing directories and their contents in one command is a pretty potent weapon (I don't think the OS offers this directly).

It would pretty much be the Godot equivalent of rm -rf. I personally don't think we should be avoiding a feature like this just because it could break stuff if used incorrectly. I would say that it is the programmer's fault for blindly using a method they don't understand the risks of. We could always add a warning to the documentation if we want to.

@Elip100 Elip100 changed the title Expose DirAccess::erase_contents_recursive() to scripting Add remove_recursive and remove_recursive_absolute to DirAccess Jul 3, 2026
@Elip100

Elip100 commented Jul 3, 2026

Copy link
Copy Markdown
Contributor Author

I've committed changes to undo exposing erase_contents_recursive and instead add remove_recursive and remove_recursive_absolute. Under the hood, these really just call erase_contents_recursive on the directory and then remove it afterwards.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Projects

None yet

Development

Successfully merging this pull request may close these issues.

Add a DirAccess method to remove a non-empty directory

4 participants