search

mdsteele / rust-cab

Rust library for encoding/decoding Windows cabinet (.cab) files
MIT License
15 stars 8 forks source link

Rewrite API #16

Closed ikrivosheev closed 1 year ago

ikrivosheev commented 1 year ago

Hello, thank you for the great library! The PR improves and makes API more ergonomic and fix problem with performance. Structure is: Cabinet -> FolderEntries -> FolderReader -> FileEntries -> FileReader.

Changes

  1. Replace actions-rs to dtolnay/rust-toolchain because actions-rs is deprecated
  2. Added FolderReader for iterate over FileReader
  3. Added FileReader for reader file from cab

Breaking changes

  1. Removed Cabinet::get_file_entry
  2. Removed Cabinet::read_file

If the idea is ok I can finish it.

mdsteele commented 1 year ago

Thanks! I'm open to updating the API, especially if it means improving performance. I'm having a little trouble following this change at a glance, but it seems like the high-level idea is that the items that FolderEntries iterates over are themselves readable, instead of only containing metadata? Am I understanding correctly?

Would it be possible to still support Cabinet::get_file_entry and/or Cabinet::read_file as high-level convenience methods, even if that's no longer the only (or most efficient) way of reading files? Just in the interest of minimizing breaking changes.

Also, since there's a lot going on here, I think the changes will be easier to review if you're able break them up into multiple smaller PRs (for example, the one you already sent to update actions-rs, maybe another for updating the edition and/or clap version, etc.)

ikrivosheev commented 1 year ago

@mdsteele thank you for the review. I have created PR:

  1. https://github.com/mdsteele/rust-cab/pull/18
  2. https://github.com/mdsteele/rust-cab/pull/17
  3. https://github.com/mdsteele/rust-cab/pull/19

After I will continue splitting this PR.