Add `GFile`, a wrapper for `FileAccess` implementing `std::io::*` traits by StatisMike · Pull Request #473 · godot-rust/gdext (original) (raw)

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 andprivacy statement. We’ll occasionally send you account related emails.

Already on GitHub?Sign in to your account

Conversation78 Commits2 Checks0 Files changed

Conversation

This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.Learn more about bidirectional Unicode characters

[ Show hidden characters]({{ revealButtonHref }})

Member

@Bromeon Bromeon left a comment • Loading

Choose a reason for hiding this comment

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

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:

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.
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.
Some docs on the class would be nice!
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.
The new file needs a license header, which is why one CI job fails.

Comment on lines 396 to 400

while bytes_written < bytes_to_write {
self.fa.store_8(buf[bytes_written]);
bytes_written += 1;
self.check_error()?;
}

Choose a reason for hiding this comment

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

Choose a reason for hiding this comment

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

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 :)

Choose a reason for hiding this comment

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

Thanks for the great update! 👍

Some comments:

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.
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?
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 😊

@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?

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.

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.

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.

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

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!

@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.

Member

@Bromeon Bromeon left a comment • Loading

Choose a reason for hiding this comment

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

Thanks! Some more comments.

Comment on lines 61 to 84

/// // Open file in write mode
/// let mut my_file = GFile::open("user://save_game.sav", ModeFlags::WRITE)
/// .expect("couldn't open file for writing");
///
/// // Write some lines into it
/// my_file.write_gstring_line("This is my saved game")
/// .expect("couldn't write first line");
/// my_file.write_gstring_line("I played for 5 minutes")
/// .expect("couldn't write second line");
///
/// // Consume object and close the file
/// drop(my_file);
///
/// // Open file in read mode
/// let mut my_file = GFile::open("user://save_game.sav", ModeFlags::READ)
/// .expect("couldn't open file for reading");
///
/// // Read lines
/// let first_line = my_file.read_gstring_line()
/// .expect("couldn't read first line");
/// assert_eq!(first_line, GString::from("This is my saved game"));
/// let second_line = my_file.read_gstring_line()
/// .expect("couldn't read second line");
/// assert_eq!(second_line, GString::from("I played for 5 minutes"));
///
/// // Consume object and close the file
/// drop(my_file);
/// ```

Choose a reason for hiding this comment

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

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

Ok(Self::from_inner(fa))
}

/// Open a compressed file

Choose a reason for hiding this comment

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

Full stops everywhere.

Ok(Self::from_inner(fa))
}

/// Creates new [`GFile`] from provided [`Gd`]<[`FileAccess`]>.

Choose a reason for hiding this comment

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

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:

/// 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.

/// Get last modified time
///
/// Get last modified time as an unix timestamp
#[doc(alias("get_modified_time"))]

Choose a reason for hiding this comment

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

Syntax should be

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

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

///
/// For more information about underlying method see
/// [Godot documentation](https://docs.godotengine.org/en/stable/classes/class\_fileaccess.html#class-fileaccess-method-get-real).
#[doc(alias("get_real", "get_float"))]

Choose a reason for hiding this comment

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

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

Choose a reason for hiding this comment

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

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).

Comment on lines 47 to 49

/// - `ModeFlags::WRITE` opens the file for write operations. If the file
/// doesn't exist at provided `path`, it is created. If it exists, it is
/// truncated after the file is closed.

Choose a reason for hiding this comment

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

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.

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...

@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.

…traits

Signed-off-by: Jan Haller bromeon@gmail.com

Bromeon changed the title~~WIP: GFile - wrapper struct for FileAccess implementing std::io::* traits~~ Add GFile, a wrapper for FileAccess implementing std::io::* traits

Nov 19, 2023

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) 🙂

Labels

c: engine

Godot classes (nodes, resources, ...)

feature

Adds functionality to the library

Add GFile, a wrapper for FileAccess implementing std::io::* traits by StatisMike · Pull Request #473 · godot-rust/gdext (original) (raw)

Conversation

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Add `GFile`, a wrapper for `FileAccess` implementing `std::io::*` traits by StatisMike · Pull Request #473 · godot-rust/gdext (original) (raw)