Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Implement distinct Flush types for Compress::compress vs Decompress::decompress #109

Merged
merged 5 commits into from Nov 7, 2017

Conversation

Projects
None yet
4 participants
@chrisvittal
Copy link
Contributor

chrisvittal commented May 7, 2017

This implements distinct flush types (FlushCompress and FlushDecompress) for Compress::compress and Decompress::decompress as in #79. It also removes the old Flush type.

To keep the code generic in read<R: BufRead, D: Ops> and Writer<W: Write, D: Ops>, this adds an internal Flush trait for abstracting over the common values of the new flush types. This new Flush trait is used as a bound for a new associated type (also called Flush) for Ops.

chrisvittal added some commits May 6, 2017

Split Flush into two enums.
The Flush enum is now FlushCompress and FlushDecompress. Each enum only
contains the valid types necessary for fuctioning of the compress and
decompress methods respectively. Also removed the Flush enum and added
a Flush trait to allow Writer to remain generic in Ops.
Clean up docs and new Flush trait.
Flush trait now only has methods for values common to FlushCompress and
FlushDecompress. Changed documentation that referenced now nonexistant
Flush enum.
@alexcrichton
Copy link
Owner

alexcrichton left a comment

Looks great to me! Just one minro comment about a little more flexibility around the enum, but otherwise I'd be ready to merge for the 1.0 release.

src/mem.rs Outdated
/// The return value may indicate that the stream is not yet done and more
/// data has yet to be processed.
Finish = ffi::MZ_FINISH as isize
}

This comment has been minimized.

@alexcrichton

alexcrichton May 8, 2017

Owner

While we're at it, both of these flush enums are intended to be extensible in the sense that client code shouldn't match on them exhaustively. To accomplish this mind adding a __Nonexhaustive variant tagged with #[doc(hidden)] to both enums? That way it should hopefully send a signal of "we may extend this in the future".

@alexcrichton

This comment has been minimized.

Copy link
Owner

alexcrichton commented May 8, 2017

👍 Thanks! I'll coordinate this with the 1.0 release so I won't merge this just yet, but rest assured I'll take it from here :)

/// This is for advanced applications that need to control the emission of
/// deflate blocks.
Block = ffi::MZ_BLOCK as isize,

This comment has been minimized.

@fstirlitz

fstirlitz Jun 24, 2017

Why was this removed? The commit message doesn't explain.

This comment has been minimized.

@alexcrichton

alexcrichton Jul 3, 2017

Owner

@fstirlitz I believe it's because it's not actually used in the backend, but do you have a use case for it?

@opilar

This comment has been minimized.

Copy link
Contributor

opilar commented Sep 8, 2017

@alexcrichton seems it's the last one already. Merge it and the crate is ready for evaluation!

@alexcrichton

This comment has been minimized.

Copy link
Owner

alexcrichton commented Sep 18, 2017

Indeed yeah thanks! I just need to allocate some time to go through the final steps of releasing 1.0 and then publish 1.0. I'll be sure to merge this before that though!

@alexcrichton alexcrichton merged commit d9291ed into alexcrichton:master Nov 7, 2017

2 checks passed

continuous-integration/appveyor/pr AppVeyor build succeeded
Details
continuous-integration/travis-ci/pr The Travis CI build passed
Details
@alexcrichton

This comment has been minimized.

Copy link
Owner

alexcrichton commented Nov 7, 2017

Alright I'm doing some prep work so I've merged, thanks so much again @chrisvittal for taking this on!

@chrisvittal chrisvittal deleted the chrisvittal:flush-refactor branch Nov 7, 2017

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.