Why do the fs and stream tokio readers not return tokio::read::ZipEntryReader

[jplatte]

Why is it that only the tokio::read::seek module has a ZipFileReader that returns tokio::read::ZipEntryReaders? That means that one has to wrap the entry reader(s) in tokio_util::compat::Compat to use them in a tokio context.

[mineichen]

I would appreciate the consolication of the two. base::ZipFileReader::new() could accept a custom, (sealed) Trait, which will be implemented for Tokio-AsyncRead and Futures-AsyncRead (plus their seek-impl). When enabling the tokio-feature, base::ZipFileReader::new() would simply accept e.g. tokio::fs::File as well and could be used everywhere.

[jplatte]

a custom, (sealed) Trait, which will be implemented for Tokio-AsyncRead and Futures-AsyncRead (plus their seek-impl)

You can't hae multiple blanket implementations (based on external traits) though. I don't think this is possible.

[mineichen]

The trait mustn't have a blanked implementation. But you're right, the two Generic-Implementations for tokio::AsyncRead and futures::AsyncRead could overlap and therefore conflict... Is this what you meant?

Maybe a 'ZipFileReader::new_tokio' would be a better choice

[Majored]

My only problem with having a new_tokio() is that we'd be returning a type where the reader is Compat<R>, which means if the library user wants to reference this type, they will have to pull in tokio-util themselves (which I want to avoid if possible).

That's why my current start on converting them over use types that shadow the base implementation but wrapping Compat<R> and the calls to .compat() internally to avoid having to pull it in.

[Majored]

@mineichen I saw a notification about your suggestion to provide type aliases which I quite like. Unsure if there was a specific reason for your deletion but from just playing around with it myself with with_tokio() methods, it's actually surprisingly ergonomic.

I've written up a short explainer: https://github.com/Majored/rs-async-zip/blob/main/src/tokio/mod.rs#L4-L18

I'm happy to settle on this unless anyone else has any suggestions.

[jplatte]

Makes sense, except the naming might be a bit confusing? I'd expect a with_foo constructor to always take at least one argument. Maybe for_tokio would be less confusing? 🤷

[Majored]

They do take the reader as a single argument but I assume you're mentioning this since the module documentation links are slightly confusing? Not sure there's anything we can do about that. Maybe omitting the parentheses helps but to me, it just looks a bit aesthetically odd without them.

[mineichen]

anyone else has any suggestions

Sorry for the confusion. I wasn't sure if I misinterpreted your post and just suggesting what you meant already...