Refactor - remove bytes crate
#41
Conversation
SzczurekYT
force-pushed
the
refactor/remove-bytes
branch
from
08b05c1 to
c0c9844
Compare
|
Fixed serde, ready for review |
| use crate::error::Error; | ||
|
|
||
| #[derive(Default, Eq, PartialEq)] | ||
| pub(crate) struct BinarySliceCursor<'a> { |
There was a problem hiding this comment.
I think that we are reimplementing just cursor here... What is the difference? If you need those methods we could use byteorder crate
There was a problem hiding this comment.
With our own impl we get much nicer errors (tried to read x bytes, had x available), instead of plain UnexpectedEof, and it is also just simpler, only the subset of functions we need.
There was a problem hiding this comment.
In that case please move it to utils and write short info about that.
But I am still not sure, errors like (tried to read x bytes, had x available) don't give me much information in context of NBT, maybe we could just use map_error and use something like tried to parse number (field name)? And in the future we can create crab_bytes or something like that
There was a problem hiding this comment.
In that case please move it to utils and write short info about that.
Info - sure.
Move - are you sure? I feel a separate file is better for a medium size type like this.
But I am still not sure, errors like (tried to read x bytes, had x available) don't give me much information in context of NBT, maybe we could just use map_error and use something like tried to parse
number (field name)? And in the future we can createcrab_bytesor something like that
You would need to add a map every time you are calling a read function. That would create a lot of mess.
In terms of context that's missing - I plan to add that too, just in another pr.
There was a problem hiding this comment.
Move - are you sure? I feel a separate file is better for a medium size type like this.
Sorry I was thinking about utils/slice_cursor.rs.
You would need to add a map every time you are calling a read function. That would create a lot of mess.
No? You are already doing something similar in slice_cursor.rs, and here you could instead of doing ? in place, just collect the errors and in one place convert and for example print only the id. There are a lot of different solutions and I don't think it is worth to reimplement something already done by 50 crates in project related to nbt. We are not obeying SoC only for printing how many bytes we were trying to read.
There was a problem hiding this comment.
No? You are already doing something similar in
slice_cursor.rs, and here you could instead of doing?in place, just collect the errors and in one place convert and for example print only the id.
I don't know what you mean / how it could be done.
There are a lot of different solutions and I don't think it is worth to reimplement something already done by 50 crates in project related to
nbt. We are not obeying SoC only for printing how many bytes we were trying to read.
Yeah, I see your point, and kind of agree, but I don't see how it could be done nicely, and also I'm not sure how the performance would be affected, it is possible it would be worse due to all the std::io stuff byteorder uses. I don't want to rewrite all of this, just to find out it is not worth it because performance is worse.
There was a problem hiding this comment.
I don't want to rewrite all of this, just to find out it is not worth it because performance is worse.
So nobody will want to refactor it in the future, and we'll end up with legacy code that is unrelated to our module.
I'll look into this today if that is not a problem for you
There was a problem hiding this comment.
I'll look into this today if that is not a problem for you
Sure
But I like the simplicity and elegance of the current solution, and it is also for some reason faster, so make sure the byteorder versions will be equally good.
also please don't push to this branch, make a separate one, it will be easier to compare
| use crate::error::Error; | ||
|
|
||
| #[derive(Default, Eq, PartialEq)] | ||
| pub(crate) struct BinarySliceCursor<'a> { |
There was a problem hiding this comment.
In that case please move it to utils and write short info about that.
But I am still not sure, errors like (tried to read x bytes, had x available) don't give me much information in context of NBT, maybe we could just use map_error and use something like tried to parse number (field name)? And in the future we can create crab_bytes or something like that
SzczurekYT
force-pushed
the
refactor/remove-bytes
branch
from
58165e2 to
ac36999
Compare
| } else { | ||
| Err(Error::NotEnoughBytes { | ||
| requested: count, | ||
| available: self.inner.len() - self.pos, |
There was a problem hiding this comment.
| available: self.inner.len() - self.pos, | |
| available: len - self.pos, |
Removes our dependency on the
bytescrate.Instead we use our own
BinarySliceCursortype, which acts as a wrapper over&[u8].The type also has checks for how many bytes are remaining which means this closes #26
For writing we now use a raw Vec, with
.extend(num.to_be_bytes()). I considered also creating a wrapper type, however I belive it is unnecesary and would be mostly a boilerplate.As a side effect of these changes we also got some pretty nice performance improvements, especially on the write side, which was pretty unexpected to me.
Question - do we want to keep the cursor methods? They were originally added as a way to avoid
byteswhich is now not needed.