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

Improve workflow of File and Directory #1551

Open
nathanfranke opened this issue Sep 23, 2020 · 7 comments
Open

Improve workflow of File and Directory #1551

nathanfranke opened this issue Sep 23, 2020 · 7 comments

Comments

@nathanfranke
Copy link

nathanfranke commented Sep 23, 2020

Describe the project you are working on:

Considering reworking much of the File and Directory workflow for 4.0, and making this issue to brainstorm and discuss what we should change.
Kind of a continuation of #1225

Describe the problem or limitation you are having in your project:

Currently, the system for files and directories is quite verbose and takes a lot of boilerplate.

Describe the feature / enhancement and how it helps to overcome the problem or limitation:

I want to take inspiration from Java's File API.

  • Each file object must already be connected to a file or directory Heart of the proposal
  • "File" can represent a file or a directory Doesn't really make sense tbh
  • Directory contents can be iterated with get_contents() Resolved in Add get_contents method to Directory class godot#40547

Describe how your proposal will work, with code, pseudocode, mockups, and/or diagrams:

# Construct the file
# File should be immediately constructed with a path
var icon = File.new("res://icon.png")
print(icon.get_path()) # res://icon.png
print(icon.open(File.READ)) # OK

Edit 2022:

  • We shouldn't pass File.READ in the constructor (removed it in example), since 1) users might not want to READ the file's contents, 2) Errors couldn't be handled.
  • Hmm, get_path and get_name are String methods, I don't think we should just copy paste each of them...
(Directory.get_contents() is implemented already, see old suggestion here)
var music_directory = Directory.new("res://Music")
# Idea: get_contents() can return a list or stream of `File`s
# Q: Should there also be a method that returns subfolders? What if the user wants both files and subfolders?
#     I don't think that there's much of a good reason to be honest since they are both different data types.
for song_file in music_directory.get_contents():
    print("Loading song file at " + song_file.get_name())
    # Load could also take a file
    var song = load(song_file)

If this enhancement will not be used often, can it be worked around with a few lines of script?:

Files and Directories are used very often

Is there a reason why this should be core and not an add-on in the asset library?:

File is already part of Godot's core

@nathanfranke nathanfranke changed the title Object-oriented approach to File and Directory Improve workflow of File and Directory Sep 23, 2020
@bruvzg
Copy link
Member

bruvzg commented Sep 23, 2020

Idea: get_contents() can return a list or stream of Files

Open each file every time folder is iterated? In this case File constructor should be lazy and do absolutely nothing until its content is accessed, which probably is not the best idea, people expect File constructor to lock files or error out immediately.

@DanielKinsman
Copy link

Given gdscript's python inspiration, could be worth looking at their newer pathlib stuff? https://docs.python.org/3.8/library/pathlib.html

@nathanfranke
Copy link
Author

@bruvzg the constructor should only fail if the path has an invalid syntax. There should be no fail for "file not found" unless you actually try to read from it.

Why? Because for example:

var file = File.new("res://abc")
if file.exists():
    pass

@jonbonazza
Copy link

jonbonazza commented Sep 24, 2020

I agree that the current file apis are cumbersome and Java's file api is a good source for inspiration.

Also, constructors should never throw an error or fail.

@h0lley
Copy link

h0lley commented Oct 12, 2020

agreed, especially the boilerplate code for iterating over the contents of a directory is really quite unwieldy:

var dir := Directory.new()
if dir.open("user://saves") == OK:
    dir.list_dir_begin()
    var file_name := dir.get_next()
    while file_name:
        if file_name.begins_with("."):
            file_name = dir.get_next()
            continue

        # do something ...
        
        continue

My initial thought about improving situations like those at least somewhat would be for GDScript to support assignment within while conditions, but I also like what's been proposed and am in favor of the consideration to combine the two APIs.

@dalexeev
Copy link
Member

Definitely need to rework the mode flags. First, it's easy to confuse READ_WRITE and WRITE_READ. Second, if, for example, you need to write data to a log file, you get the following code:

var f = File.new()
var mode = f.READ_WRITE
if !f.file_exists():
    mode = f.WRITE
f.open("user://path", mode)
f.seek_end()

I suggest replacing these flags with READ, WRITE, CREATE, APPEND (or SEEK_END?), TRUNCATE.

iterating over the contents of a directory is really quite unwieldy

I just use the following pattern:

var dir = Directory.new()
dir.open("res://path/")
dir.list_dir_begin(true) # skip_navigational
while true:
    var fname = dir.get_next()
    if !fname: break
    if !fname.match("*.ext"): continue
    
    ...

Firstly, the entire first file is now processed in a loop, like all the others, and secondly, since all processing is at the beginning of the loop, I can safely use continue further in the loop body if necessary.

var file = File.new("res://abc")
if file.exists():
    pass

In principle, this is also acceptable, only we need a different method, for example file.is_open(), since the error can be of a different type (for example ERR_FILE_NO_PERMISSION ). However, this way does not give a gain in the number of lines.

But, in built-in classes, usually .new() takes no arguments. I don’t know what it’s connected with, but it is definitely true.

@nathanfranke
Copy link
Author

Updates after a couple years:

(P.S.: Can a maintainer rename godotengine/godot#40547 to specify both get_files and get_directories? get_contents is only a private, internal method)

I think we'll need an expert to take a look at the suggested API. Despite being the proposal author, I am not fully convinced it is how things should be.


var file = File.new("res://abc")
if file.exists():
    pass

Hmm, I don't think this is good (I know my proposal implied it though). I can see users doing something like

var file = File.new(...)
if file.exists():
	file.open(...)

Which can cause race conditions (as well as permissions issues in some cases).

The user should do this:

var file = File.new(...)
if file.open(...) == OK:
	# now we can read stuff!

Whether File.new(...) actually specifies the path is up to discussion.

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

No branches or pull requests

7 participants