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

Change interface to use individual file objects #163

Open
2ndDerivative opened this issue Nov 23, 2023 · 3 comments
Open

Change interface to use individual file objects #163

2ndDerivative opened this issue Nov 23, 2023 · 3 comments
Labels
enhancement New feature or request

Comments

@2ndDerivative
Copy link

In terms of the API of this crate, one of the simplest use cases (writing a zip file) with (admittedly not well-formatted but easily imaginable) code like this has a footgun:

fn zip_files(paths: &[PathBuf]) -> Result<Vec<u8>, ZipError> {
    let mut io_cursor = Cursor::new(Vec::<u8>::new());
    let mut zipwriter = zip::ZipWriter::new(&mut io_cursor);
    let options = FileOptions::default().compression_method(zip::CompressionMethod::Stored);
    for path in paths {
        let mut buffer = vec![];
        zipwriter.start_file(path.to_str().unwrap(), options)?;
        let mut filehandle = std::fs::File::open(path)?;
        filehandle.read_to_end(&mut buffer)?;
        zipwriter.write_all(&buffer)?;
    }
    zipwriter.finish()?;
    // Code does not compile without this
    // drop(zipwriter);
    Ok(io_cursor.into_inner())
}

Without an active drop(), this code does not compile, as the zipwriter keeps the exclusive reference even though it's finished.
This seems unnecessary, since using the writer after completing the zip file is explicitly not recommended behaviour anyway.
This would not be a problem if zipwriter.finish() simply consumed self, which could also drop it after finish() is completed.

This could be solved with an own scope too, but that is obviously somewhat equivalent to the active drop() invocation.
Obviously this would be a breaking change, but I imagine this could be a worthwhile improvement to the API of ZipWriter

@enkore
Copy link

enkore commented Feb 19, 2024

I'm not madly in love with the overall ZipWriter API. Having start_xxx() and finish_xxx() pairs smells like it should be a separate struct. Having start_xxx() consume self and return an io::Write impl, which in turn has a finish() method consuming self and returning the original ZipWriter would categorically prevent logic bugs where you interleave writes to different files. And it would be immediately obvious from the API that you can only write at most one file at a time.

@Pr0methean
Copy link
Member

In zip 1.x, ZipWriter::finish does return the inner writer. I'll look into creating objects for individual files.

@Pr0methean
Copy link
Member

Related: #98

@Pr0methean Pr0methean transferred this issue from zip-rs/zip-old Jun 2, 2024
@Pr0methean Pr0methean changed the title Make ZipWriter.finish() consume ZipWriter Change interface to use individual file objects Jul 21, 2024
@Pr0methean Pr0methean added the enhancement New feature or request label Jul 27, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement New feature or request
Projects
None yet
Development

No branches or pull requests

3 participants