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

Directory and Error extension methods #1

Open
wants to merge 2 commits into
base: master
Choose a base branch
from

Conversation

Carnagion
Copy link

I have added extension methods for Directory and Error.

DirectoryExt.GetFiles() and DirectoryExt.GetDirectories() do what their System.IO.Directory counterparts do, and thus help make Godot's filesystem API less of a pain to work with.

ErrorExt.Success() makes it possible to check whether an error occurred or not, without having to use != or is not all the time.
ErrorExt.ThrowIfFailed() makes it easy to throw an exception (and therefore terminate the application) if an Error is returned by Godot, without having to use an if statement and compose an error message manually.

@Carnagion
Copy link
Author

After a bit of digging around, I have found a few issues with the Directory extension methods:

  1. As @derkork pointed out earlier, due to the laziness of GetDirectories() and GetFiles(), the method will exit early if used with a break in a foreach loop. In such cases, ListDirEnd() will never be called on the Directory objects, leaving them in an invalid state.

  2. The inner Directory objects used in GetDirectories() and GetFiles() are not disposed of. A simple using declaration will not suffice, as once again the laziness of the methods causes the Directory to be disposed before the results of GetDirectories() or GetElementsNonRecursive() are enumerated, thereby throwing an exception.

Taking into account the discussion on Discord and the potential use cases of these methods, my opinion is that the best solution would be to have these return a string[] rather than IEnumerable<string>, and also to include async versions that make use of godot-ext's SceneTree.NextFrame() extension method to retrieve the results on a per-frame basis.

Such an implementation would take care of the issues arising from the laziness, while also giving users the choice to use either a blocking method or an asynchronous method.

This is not without its own problems though. As mentioned in the discussion, the asynchronous method would still take a long time to finish in the case of hundreds or thousands of results being retrieved. However, I do not consider this to be a major problem. For one, it is unlikely that a user has thousands of files or subdirectories, and would want to query all of their paths. Even if they do, there are other ways to reduce the time taken by either method, such as by rearranging the folder structure to ensure that files or directories whose paths don't need to be retrieved at the same time are kept under different parent directories. The user could also simply go for their own implementation of such a method in cases where they would require fine-grained control over the way the query is performed.

What are your thoughts @derkork? This can be discussed at length further if necessary.

@derkork
Copy link
Owner

derkork commented Jun 18, 2022

Well given the amount of "if"s surrounding this proposal I am not sure whether this can be implemented in a sufficiently generic way that is universally useful and doesn't offer ways for the developers using it to shoot themselves in the foot. Maybe this is something that depends too much on the use case and has no generic implementation.

@Carnagion
Copy link
Author

Well given the amount of "if"s surrounding this proposal I am not sure whether this can be implemented in a sufficiently generic way that is universally useful and doesn't offer ways for the developers using it to shoot themselves in the foot. Maybe this is something that depends too much on the use case and has no generic implementation.

Yeah, this is turning out to be something for which a generic catch-all solution doesn't seem to exist. Using string[] instead of IEnumerable<string> has turned out to be quite useful for me, and it certainly works in most "normal" scenarios - but then again, I understand that extension methods such as these should work for almost all scenarios, not just "most" of them.

I'll keep thinking of alternative solutions in the meantime.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants