Rename internal types to match the public types about flate2-rs HOT 16 CLOSED

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?

from flate2-rs.

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;
}

from flate2-rs.

@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

from flate2-rs.

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

from flate2-rs.

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

from flate2-rs.

(oops didn't mean to close)

from flate2-rs.

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.

from flate2-rs.

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 :(

from flate2-rs.

@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?

from flate2-rs.

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

from flate2-rs.

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.

from flate2-rs.

from flate2-rs.

Looks like this is the only 1.0 blocker left.

from flate2-rs.

@opilar are you still making progress on this?

from flate2-rs.

@dtolnay sorry. No.

from flate2-rs.

Fixed in #120

from flate2-rs.