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

Add get_contents method to Directory class #40547

Merged
merged 1 commit into from
Dec 10, 2021

Conversation

KoBeWi
Copy link
Member

@KoBeWi KoBeWi commented Jul 20, 2020

Closes godotengine/godot-proposals#1225
Resolves #55776

Thing to consider:

  • this could be theoretically used on an arbitrary directory without opening it (similar to Dir.entries in Ruby)
  • this makes list_dir_begin() and related methods unnecessary, as they are less convenient

Theoretically list_dir_begin() is more optimal to use when you want to cancel iteration midway, but this happens very rarely and the difference is minimal.

@KoBeWi KoBeWi added this to the 4.0 milestone Jul 20, 2020
@KoBeWi KoBeWi requested a review from a team as a code owner July 20, 2020 17:28
@nathanfranke
Copy link
Contributor

+1 for get_contents instead of get_files. Better not to argue about whether folders are considered "files" and just be explicit. :)

@dreamsComeTrue
Copy link
Contributor

Would you be considering adding recursive parameter too? :)

@KoBeWi
Copy link
Member Author

KoBeWi commented Jul 22, 2020

Would you be considering adding recursive parameter too? :)

I started implementing it, but the code feels like it would be simpler in GDScript. The additional complexity is not worth it IMO.

@nathanfranke
Copy link
Contributor

#40763

There might be a redesign for 4.0 which may affect whether or not this is merged.


As for recursion, how would that even work? Are directories included in the list or only the files within them?

@Xrayez
Copy link
Contributor

Xrayez commented Jul 31, 2020

Recursive option is probably not worth it because you'll likely also want to add some filtering on top of that, which is better done via script then... given the development philosophy of Lean & Clean™. 🙂

@mhilbrunner
Copy link
Member

mhilbrunner commented Dec 7, 2021

PR meeting: the bool args should be properties (this should be refactored in core binds for all directory stuff), and it should likely be added as seperate methods to list files/directory
Otherwise this looks good :)

@KoBeWi KoBeWi force-pushed the directory_inspectory branch from 7c4791e to dd67119 Compare December 7, 2021 17:20
@KoBeWi KoBeWi requested a review from a team as a code owner December 7, 2021 17:20
@KoBeWi
Copy link
Member Author

KoBeWi commented Dec 7, 2021

Rebased and done requested changes.
Additionally show_navigational is now named include_navigational (same with hidden).

Copy link
Member

@akien-mga akien-mga left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

See #55776 - the listing is not deterministic, but it could be sorted here in get_contents(). And you could add a mention about list_dir_begin not guaranteeing order in the docs to close that issue.

@KoBeWi KoBeWi force-pushed the directory_inspectory branch from dd67119 to 2a2ccf3 Compare December 10, 2021 16:05
@KoBeWi KoBeWi force-pushed the directory_inspectory branch from 2a2ccf3 to d04c2a5 Compare December 10, 2021 16:24
@akien-mga akien-mga merged commit bc6cd53 into godotengine:master Dec 10, 2021
@akien-mga
Copy link
Member

Thanks!

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