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 try_into_inner() for ISO9660 and FileRef #2

Open
wants to merge 1 commit into
base: master
Choose a base branch
from

Conversation

losynix
Copy link
Contributor

@losynix losynix commented Feb 10, 2022

Hello !

It would be nice to get ownership of the inner reader back when we're done. Not sure this is the best approach, FileRef makes this tricky but it works for me.

@ids1024
Copy link
Owner

ids1024 commented Feb 15, 2022

Not sure this is the best approach, FileRef makes this tricky but it works for me.

Hm, yeah. A try_into_inner function seems a bit odd to have, but I can see how this could be useful.

I'm not entirely happy with how reference counting is used in the crate in general, but I'm not sure the most efficient and ergonomic Rust API for ISO9660 (and filesystems generally; though is simpler in that it's read only).

@ids1024
Copy link
Owner

ids1024 commented Feb 15, 2022

Read is implemented for &mut T where T: Read, so where lifetime bounds aren't inconvenient, you can use something like ISO9660::new(&mut file) without consuming the original reader.

@losynix
Copy link
Contributor Author

losynix commented Feb 28, 2022

I'm not sure what the best approach for a filesystem API would be either. Other crates I checked like rust-fatfs or ext4-rs take ownership of the inner reader whereas ntfs asks for the reader in every functions accessing the filesystem.

Anyway you're right I can probably get around this by giving a reference to ISO9660::new(), thank you.

@losynix losynix force-pushed the into_inner branch 2 times, most recently from 7a2bb3e to 895f3d4 Compare April 3, 2023 08:39
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