Add GFile, a wrapper for FileAccess implementing std::io::* traits

[StatisMike]

Resolves #446

[GodotRust]

API docs are being generated and will be shortly available at: https://godot-rust.github.io/docs/gdext/pr-473

[Bromeon]

Thanks a lot, this is a great QoL addition for users! 🚀
Here is the doc link, see also bot comment.

Added lots of comments in the code, some general remarks:

1. This should be in godot::engine module, not godot::builtin. It's not great how all classes, traits and sub-modules are mixed together, but I'm also not really aware of a better way without deep nesting.
2. Make sure the module appears only once in public API (or twice including prelude). At the moment, it's public in both builtin and builtin::file.
3. Some docs on the class would be nice!
4. For function docs, I would usually have a 1-liner which gives a very high-level but useful overview of the function does. Then there's an empty line, and after that (in the "body") you can go into details, caveats, related functions, etc.
5. The new file needs a license header, which is why one CI job fails.

[Bromeon]

Note: writing individual bytes is very inefficient, I think in the future we should use this method: https://docs.godotengine.org/en/stable/classes/class_fileaccess.html#class-fileaccess-method-store-buffer

We could keep a PackedByteArray allocated with our buffer size. Maybe we then don't even need a Vec. Same for reading.

Anyway, we should do this in a separate PR; I think it's great to get this initial version working first before optimizing 🙂

[StatisMike]

If there would be a move to store_buffer for write, the flush() will potentially also need its implementation. But as you said - its a topic for the future :)

[Bromeon]

Thanks for the great update! 👍

Some comments:

1. Please double-check sentences for capitalization at beginning, full stop at end, and typos. These may sound like annoying little things, but they notably increase documentation quality and consistency.
2. Thanks for the GdRefUniqueness! It's a nice addition, I might refactor it a bit in the future depending on other needs. Could you name it NotUniqueError for now?
3. I'm going to rename GodotString to GString tomorrow, as well as strip some get_ prefixes, which might make your PR run into deprecations/errors. Is it OK if I update it accordingly with a separate commit? You're spending enough work on this feature, so I don't want you to deal with merge conflicts in addition 😊

[StatisMike]

@Bromeon AD NotUniqueError:

• keep it in gfile.rs, move it to engine/mod.rs, or some separate engine::errors module?
• expose it as godot_core::engine::NotUniqueError and godot::prelude::NotUniqueError?

[Bromeon]

keep it in gfile.rs, move it to engine/mod.rs, or some separate engine::errors module?

You can keep it there for now, I'll find a better place at some point 🙂

expose it as godot_core::engine::NotUniqueError and godot::prelude::NotUniqueError?

Only the first one, it's not frequent enough to warrant prelude.

[StatisMike]

I've added aliases for docs, handled the errors in doc building and (I hope) the error in itest on Windows. Reformatted the docs to try to uphold the 80-character line general convention, if other should be applied to this crate please add information to Contributing.md (or link to the other file containing format guidelines, as I haven't really found one).

If there is a need to squash commits on the contributor side, let me know - I've seen there should be an option for maintainers to squash commits during merge on GitHub, which could be potentially safer (every time I type rebase I get cold sweats 😝 ).

Feel free to resolve conflicts with GodotString->GString rename after merging the rename change.

[Bromeon]

If there is a need to squash commits on the contributor side, let me know - I've seen there should be an option for maintainers to squash commits during merge on GitHub, which could be potentially safer

From the Contributing guideline:

Since we use GitHub merge queues, we can unfortunately not decide to squash commits upon merge per PR.

But no worries, I can do it as part of the rename, so don't worry about it 🙂
In the future, you can just use git commit --amend to modify previous commit, followed by git push --force-with-lease.

[StatisMike]

There was a wrong path to the GodotClass in one of the doc - I omitted calling the cargo doc... Everything should be an ok now.

Edit: I knew I would botch something with amending... It seems like the amend before were pushed incompleted

[Bromeon]

I squashed all commits and updated the PR to work with latest API. Since the decision around get_ prefixes is pending and might be reverted, I didn't want to rewrite your code just to potentially rewrite it again. Instead, I added a 2nd commit with a temporary workaround.

That said, it would be nice if you could address my other suggestions, e.g. line lengths, failing doctests and doc links, and more. Thanks!

[StatisMike]

@Bromeon There seemed my git incapability on my fork that affected my work on another branch worked its way into this too: doc reformat, doc tests and module rename (file -> gfile) all weren't present after my pull. Amended it all, also included the renames for get_* to work on current state of crate, to check the CI here.

Not too many changes there were regarding get_* rename, so after the decision will be made I will just amend commit myself, if needed.

[Bromeon]

Thanks! Some more comments.

[Bromeon]

What if instead of all these expect statements, you use ? inside two functions save_game() and load_game()?
That would:

• make the code much more readable
• only show the relevant parts (namely the GFile API)
• make explicit drop unnecessary, that's rather unusual in Rust anyway

[Bromeon]

Full stops everywhere.

[Bromeon]

Doc link doesn't work, also elsewhere. There's no need to repeat the types, users can click on them in the parameter list.

I'd rather have a brief description such as:

Suggested change

	/// Creates new [`GFile`] from provided [`Gd`]<[`FileAccess`]>.
	/// Creates new [`GFile`] from a `FileAccess` pointer with a reference count of 1.

Also, name should be try_from_unique, in case we later want to add a from_unique that panics.

[Bromeon]

Syntax should be

Suggested change

	#[doc(alias("get_modified_time"))]
	#[doc(alias = "get_modified_time")]

according to docs (and it's also how we do it elsewhere).

[Bromeon]

Is get_real not depending on whether Godot is compiled in single- or double-precision format?

[Bromeon]

Thanks! There is clippy failing, but other then that, functionality-wise it looks good!

There's mostly nitpicks and doc quality issues left. Comment line lenghts are still not good, already mentioned here. You don't need to use the entire 140 chars, but definitely much more than the current 80 (ideal 120-140).

[Bromeon]

If it exists, it is truncated after the file is closed.

Why "after the file is closed"? Isn't it overwritten immediately?

Same for WRITE_READ below.

[Bromeon]

What's happening now? You're undoing improvements, and there are now both gfile.rs and file.rs 😅

I think you should reset hard to commit 94e44d5, and take it from there...

[StatisMike]

@Bromeon That is a long personal journey, and PR comments don't seem like a good place for it. The good thing is that at the end of it, I will end up being much more literate in git and a much less troublesome contributor (I hope).

Will fix this up, afterwards will wait for you to merge #487 and fix things up with it in place.

[Bromeon]

Thanks so much for the repeated improvements and the patience with Git and my nitpicks! 😀

This contribution has reached a great quality, it's ready to merge. I rebased it onto master and made smaller adjustments to docs. It will likely see some more changes after merge (e.g. regarding the unique invariant) 🙂