feat: add ZipFileData to public API#860
Conversation
There was a problem hiding this comment.
Pull request overview
Note
Copilot was unable to run its full agentic suite in this review.
This PR updates the crate’s public API exports by re-exporting an additional type from the read module.
Changes:
- Re-export
ZipFileDataalongsideHasZipMetadatafromcrate::read.
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| pub use crate::format::aes::AesMode; | ||
| pub use crate::format::flags::System; | ||
| pub use crate::read::HasZipMetadata; | ||
| pub use crate::read::{HasZipMetadata, ZipFileData}; |
There was a problem hiding this comment.
Code Review
This pull request updates the public exports in src/lib.rs to include ZipFileData via the read module. The reviewer identified that this change will result in a compilation error because ZipFileData is not currently exported from the read module, and provided a code suggestion to export it directly from crate::types instead.
Important
The consumer version of Gemini Code Assist on GitHub is being sunset. Starting June 18, 2026, new organization installations will be blocked, and all code review activity will officially cease on July 17, 2026.
For more details on the timeline and next steps, please review the Help Documentation.
| pub use crate::format::aes::AesMode; | ||
| pub use crate::format::flags::System; | ||
| pub use crate::read::HasZipMetadata; | ||
| pub use crate::read::{HasZipMetadata, ZipFileData}; |
There was a problem hiding this comment.
This change will cause a compilation error because ZipFileData is not publicly exported from the read module (crate::read). In src/read/mod.rs, ZipFileData is only imported privately via use crate::types::ZipFileData;.
To fix this, you can either:
- Re-export
ZipFileDatadirectly fromcrate::typesinsrc/lib.rs(consistent with howDateTimeorSystemare exported). - Or, if you want it to be accessible via
zip::read::ZipFileDataas well, you must also modifysrc/read/mod.rsto publicly export it (pub use crate::types::ZipFileData;).
| pub use crate::read::{HasZipMetadata, ZipFileData}; | |
| pub use crate::read::HasZipMetadata; | |
| pub use crate::types::ZipFileData; |
Hi, Which field do you need to access ? Your function could take a |
|
The issue with ZipFile is that it's generic over the reader, which massively complicates the closure's trait bounds. |
|
And which field do you need to access ? |
|
As for my use case, I currently just want the name of the file, be it UTF-8 or not. |
Are you not using https://docs.rs/zip/latest/src/zip/read.rs.html#808-810 btw in the next version, the name will not be part of Better start using |
|
Yeah, what I'm doing now is restricting filters to UTF-8. The closure just provides a &str. But that's both somewhat likely to need to change in the future, which I'd like to avoid (especially if something like this makes it into my own public API), and also not as clear as "you get the archive's metadata" and requires a bit more documentation. |
You can do use zip::HasZipMetadata;
fn filter(zip_file: &impl HasZipMetadata) -> bool {
// a random condition
zip_file.get_metadata().crc32 > 0
}
fn main() {
use std::io::Cursor;
use std::io::Write;
use zip::CompressionMethod::Stored;
use zip::ZipArchive;
use zip::ZipWriter;
use zip::write::SimpleFileOptions;
let mut writer = ZipWriter::new(Cursor::new(Vec::new()));
let options = SimpleFileOptions::default().compression_method(Stored);
let filename = format!("test.txt");
writer.start_file(&filename, options).unwrap();
writer.write_all(b"content").unwrap();
// Write and read back
let bytes = writer.finish().unwrap().into_inner();
let mut reader = ZipArchive::new(Cursor::new(bytes)).unwrap();
let file = reader.by_index(0).unwrap();
if filter(&file) {
println!("OK");
}
println!("DONE");
} |
|
Yeah, but we can't have |
use zip::HasZipMetadata;
fn main() {
use std::io::Cursor;
use std::io::Write;
use zip::CompressionMethod::Stored;
use zip::ZipArchive;
use zip::ZipWriter;
use zip::write::SimpleFileOptions;
let mut writer = ZipWriter::new(Cursor::new(Vec::new()));
let options = SimpleFileOptions::default().compression_method(Stored);
let filename = format!("test.txt");
writer.start_file(&filename, options).unwrap();
writer.write_all(b"content").unwrap();
// Write and read back
let bytes = writer.finish().unwrap().into_inner();
let mut reader = ZipArchive::new(Cursor::new(bytes)).unwrap();
let filter = |z: &dyn HasZipMetadata| {
z.get_metadata().crc32 > 0
};
let file = reader.by_index(0).unwrap();
if filter(&file) {
println!("OK");
}
println!("DONE");
} |
|
Haha, yes, fair! |
Hello, I noticed that
HasZipMetadatareturns an unnamed type, making it inconvenient to pass around separately from the underlyingZipFile.My use case is filtering files based on their metadata before retrieving their contents from the archive. I'd like to have a function that accepts
impl Fn(&ZipFileData) -> bool.I see that there is a test,
test_explicit_system_roundtrip, which mentionsZipFileDatabeing unnamed in the public interface. On the other hand the name is about the only thing not public aboutZipFileData, e.g. one is free to create new instances of it viaDefault. So I fail to infer whether it being unnamed is really intended.Apologies for the brevity of the PR; hopefully the change is small enough that accepting/rejecting/requesting changes is little effort.