Re: [pve-devel] [PATCH proxmox v3 01/13] io: introduce RangeReader for bounded reads

[Thomas Lamprecht <t.lamprecht@proxmox.com>]

There's zero actual commit message and basically no rustdoc comment for
the public interface, that needs to improve, especially library crates
should be held to higher standards in this regard.

non-exhaustive list of things that I'd find relevant for such crates:
- describing the background in the commit message (where this will be used,
  why is it worth having, can we, e.g., replace some existing code with this)
- short overview as rustdoc comment, here either module wide or on the type.
- adding a (short) rustdoc example for how this would be used.
- some basic unit tests, as the Read trait is implemented for &[u8], this
  would be doable here without IO side effects for the underlying build
  system and make review of edge cases much easier.

I know you're a bit in a rush to advance this series, and we do not have
to get this fine-polished for initial inclusion; but investing roughly a
half an hour consisting of 10 minutes for a _bit_ more rustdoc and 20
minutes for some very basic tests and might get us a long way here

Am 09.07.25 um 14:34 schrieb Filip Schauer:
> diff --git a/proxmox-io/src/range_reader.rs b/proxmox-io/src/range_reader.rs
> new file mode 100644
> index 00000000..307bfe27
> --- /dev/null
> +++ b/proxmox-io/src/range_reader.rs
> @@ -0,0 +1,94 @@
> +use std::io::{Read, Seek, SeekFrom};
> +use std::ops::Range;
> +
> +pub struct RangeReader<R: Read + Seek> {
> +    reader: R,
> +
> +    /// Range inside `R`.
> +    range: Range<u64>,
> +
> +    /// Relative position inside `range`

nit: Inconsistent use of trailing sentence point, one above got one but this
here and below doesn't.

> +    position: u64,
> +
> +    /// True once the initial seek has been performed
> +    ready: bool,

nit: It's internal, so not _that_ important, but might be slightly nicer to be
very explicit with the name, e.g. seeked_once or first_seek_done, as ready
is generic and opaque, people can interpret this widely different, and while
we at least got some rustdoc here (thx to Wolfgang including it in his
suggestion to add this in his review for the previous version I guess ;), it's
still nice to have that communicated in the code that uses this.




_______________________________________________
pve-devel mailing list
pve-devel@lists.proxmox.com
https://lists.proxmox.com/cgi-bin/mailman/listinfo/pve-devel