Closed icrayix closed 1 year ago
Hey, thanks for the contribution. I'll give this a look when I get chance.
With the file issue, we can probably
FileLike
for the set_len
method File
FileLike
. This might cause some issues though with conflicting impls.
It looks to me like you can do this:
(in region.rs
)
impl<S> Region<S> where S: FileLike {
pub fn remove_chunk(&mut self, x: usize, z: usize) -> Result<()> {
todo!()
}
}
/// FileLike is a sealed trait (users cannot implement it for their types). This
/// allows us to add methods to this trait without breaking backwards
/// compatibility.
pub trait FileLike: private::Sealed {
fn set_len(&self, size: u64) -> io::Result<()>;
}
impl FileLike for File {
#[inline(always)]
fn set_len(&self, size: u64) -> io::Result<()> {
self.set_len(size)
}
}
pub(crate) mod private {
use std::fs::File;
pub trait Sealed {}
impl Sealed for File {}
}
Then because the Sealed
trait is effectively pub(crate)
, you can create a fake file type to do easier unit testing:
(in test/region.rs
)
struct FakeFile;
impl crate::region::private::Sealed for FakeFile {}
impl FileLike for FakeFile {
fn set_len(&self, size: u64) -> std::io::Result<()> {
todo!()
}
}
It might be a bit of a pain to also implement Read
, Write
, and Seek
though.
I have to admit that that's a pretty neat solution :)
The only question remaining for me is whether we should have remove_chunk for non-file regions as well. Not sure on how to implement the truncate functionality on FileLike only tho.
The only question remaining for me is whether we should have remove_chunk for non-file regions as well. Not sure on how to implement the truncate functionality on FileLike only tho.
That's a good point. We won't be able to have a remove_chunk
method on both impl
blocks as they will conflict.
edit: see comment below.
And yeah the sealed trait is cool. :) See the API guidelines.
Thinking about it some more, I'm wondering if Region
should instead expose the logical end of the stream, eg via a len
method that returns the number of bytes taken up by the region data.
This means that if you were working with a File
, you would do something like...
let region = Region::new(f);
// manipulate region a bunch
let len = region.len();
let f = region.into_inner();
f.set_len(len);
This would mean that other types like File
and follow a similar pattern for their specific type.
Even better, we could change into_inner()
to seek to the logical end rather than 0
, and a user can simply do:
let region = Region::new(f);
// manipulate region a bunch
let mut f = region.into_inner();
let len = f.stream_position();
f.set_len(len);
I think into_inner()
seeking to the end rather than 0
makes more sense anyways. I can bump to 0.27.0 to indicate the breaking change.
But doesn't into_inner consume self? One wouldn't be able to continue using the region, but rather create a new one, right? Edit: Now that I think of it again one would solely really need to access the inner stream once they have finished working on the region using the higher-level apis, so that shouldn't be an issue neither :)
Other than that I really like that approach better as the exposed api is a lot more consistent and the FileLike
trait adds a lot of confusion too.
Figuring out the logical end of the stream could get quite tricky though as the offsets tell nothing about the chunk size. Edit: Just remembered that the length of the chunk is also included in the chunk's payload, so no problem there :)
Yep, you've got it. The FileLike
definitely seems ugly in comparison.
Be aware that Minecraft likes chunks to be padded to a sector length (see #75), so we don't want to truncate the file to the exact end of the chunk data.
Here we go. A lot more verbose than expected but couldn't find any better way.
The function doc could perhaps be improved but my english skills are lacking for that.
Like that way better than the solution before :)
This looks good. I'll give it a closer look when I get chance. Thanks again! :)
Hi,
have you already taken a look into it? Not willing to annoy you just wanted to make sure you still have that on your radar :)
I have not, sorry! I had a quick look over now and it looks good, so I'm going to go ahead and merge. Thank you :)
Okay, tried to fix a build issue with the suggest feature and I made a mess of it!
There's a build failure that needs fixing, basicallly just commenting a line of code in the doccomment where cargo test
is failing. If you change that I can merge. :)
Ooops.
No clue on why I used file.truncate there even though that method doesn't even exist lol.
I also addressed a clippy warning there.
Unfortunately I did not find a way to truncate the buffer without having access to an underlying
File
or so. Maybe add animpl<File> Region
so we can useset_len
?Note: I derived
Clone
on Region as the testdeleted_chunk_at_end_of_buffer_truncates_buffer
usesinto_inner
which consumes self. Please tell me if there's any better way to do soPS: I'm pretty new to Rust so please don't hesitate to correct mistakes and improve the code :)