Rename internal types to match the public types

[dtolnay]

The write/read/bufread modules all repeat the same type names:

pub use gz::EncoderReader as flate2::read::GzEncoder;
pub use deflate::EncoderReader as flate2::read::DeflateEncoder;

Unfortunately the rustdoc for flate2::read::GzEncoder shows the private names:

impl<R: Read> EncoderReader<R>
fn new(r: R, level: Compression) -> EncoderReader<R>

This is a rustdoc bug but even aside from that, this pattern is confusing to people browsing the code because what they see clicking through a [src] link does not match how they will be using the library. Let's try to make the real type names match the public names they are exported as.

Relevant API guideline: rust-lang/api-guidelines#5
Rustdoc bug: rust-lang/rust#41072

[opilar]

I think it would be neat to have the same name for the same functionality but for different modules:

read::deflate::Encoder
read::zlib::Encoder
read::gz::Encoder

So the module declaration would be:

pub mod read {
    pub mod deflate;
    pub mod zlib;
    pub mod gz;
}

[alexcrichton]

@opilar yeah that seems plausible, but other compression/decompression formats probably wouldn't want to mirror that (e.g. read::xz::Decoder). Basically for consistency across crates (e.g. bzip2, xz2, brotli2, etc) I'd favor dropping the inner deflate, zlib, and gz modules here

[opilar]

@alexcrichton so all encoders and decoders goes to one lib.rs file? That's not good.

[alexcrichton]

Oh no I think they'll be in src/read.rs, src/write.rs, and src/bufread.rs

[alexcrichton]

(oops didn't mean to close)

[spencerwilson]

Hi! I'm interested in working on this (it'll be my first Rust OSS contribution).

After reading through the code, I have a couple questions. Considering that existing structs can't be renamed to their exposed names without colliding (e.g., gz::EncoderReader and gz::EncoderWriter are both exposed as GzEncoder), then understand the goal to be to restructure the modules {deflate,zlib,gz}.rs -> {read,bufread,write}.rs like you just described. If that's correct, where would you like the Builder and Header structs, and the test submodule, within each of the existing modules to live?

I poked around your other compression crates to try and find a convention but I found none. I was thinking perhaps either

1. Keep the format-specific modules but only have them contain the builder and header code; have them import from the new read/bufread/write modules. And I suppose all read tests would live in read.rs, and so forth?
2. Have additional modules headers.rs, builders.rs, tests.rs.

...or others I'm not thinking of. What do you think?

P.S. For what it's worth I see the convention in your other crates with respect to module structure, so I think I understand the motive for this Issue. However it almost seems worth going against the other crates' internals conventions because of how this crate contains three distinct (albeit related) compression formats, and because efforts to make this crate structurally similar to the others (i.e., by way of either of the options above) aren't so pretty. I'm interested to know what you all think.

[alexcrichton]

Thanks for picking this up @sw12!

I think the header types are ok to keep living in a (private) gz.rs submodule, they're just reexported at the root of the crate. The main problem with the other types today is that they're renamed on reexport, which is a no-no wrt rustdoc.

For tests I'd just jettison them all from the library anyway. At this point they'd probably all be best as tests/{deflate,zlib,gz}.rs separate from the library crate itself.

But yeah juggling the three similar-but-different-enough formats is a bit of a pain :(

[spencerwilson]

Cool. I've made a bit of progress--lots of deleting, replacing, deleting, etc.; for my future self's sake, what's the fastest way to do this kind of refactoring that you know of? What would be your procedure, roughly?

In any case, my branch is here. cargo build passes (and tests still need to be updated), but only when I have #![deny(missing_docs)] commented out. The reason is that when I separate the structs into different modules (bufread, read, write), code which formerly relied on being able to directly refer to private struct fields (e.g., GzEncoder's inner, head, pos, and eof) breaks. That lead me to resolving the error by making the requisite fields pub, but... is there a better way?

[alexcrichton]

@sw12 ah that's unfortunate, sorry about that! This is a problem that pub(restricted) is intended to solve, but that's unfortunately not stable just yet! With such a feature we'd tag these fields as pub(crate) so the entire crate can access them but not external users. We definitely want to make sure they're private in the long run b/c we don't want them to be part of the public interface!

I think these errors are primarily related to read accessing the internals of the bufread module, right? If that's the case we can perhaps do something like:

• Create a private module, bufread_
• Move definitions of all flate2::bufread types into bufread_
• Reexport all types from bufread_ inside of bufread
• Add top-level public functions to bufread_ which access the internals of the types
• Implement read types using these top-level functions

That way the top-level functions stay private to the crate (they're in a private module). Does that make sense?

[opilar]

@sw12 how everything is going? I may would like to help you with your contribution.

[brson]

Looks like this has stalled. As far as I can tell we just need to do what @alexcrichton recommended above. @opilar feel free to do so if you like.

[spencerwilson]

Hi all. My sincere apologies for the unresponsiveness. Yes, Michael, you're welcome to pick up where I left off if you're interested :)

…

On Jun 2, 2017 8:21 PM, "Brian Anderson" ***@***.***> wrote: Looks like this has stalled. As far as I can tell we just need to do what @alexcrichton <https://github.com/alexcrichton> recommended above. @opilar <https://github.com/opilar> feel free to do so if you like. — You are receiving this because you were mentioned. Reply to this email directly, view it on GitHub <#75 (comment)>, or mute the thread <https://github.com/notifications/unsubscribe-auth/AFXRM00tLfvfj9pB8yUb6lAIfSz1Ht2Yks5sANErgaJpZM4Mzd7P> .

[brson]

Looks like this is the only 1.0 blocker left.

[dtolnay]

@opilar are you still making progress on this?

[opilar]

@dtolnay sorry. No.

[alexcrichton]

Fixed in #120