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

Rewrite API #16

Closed
wants to merge 3 commits into from
Closed

Conversation

ikrivosheev
Copy link
Contributor

@ikrivosheev ikrivosheev commented Mar 22, 2023

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.

@ikrivosheev ikrivosheev marked this pull request as draft March 22, 2023 14:01
@ikrivosheev ikrivosheev marked this pull request as ready for review March 23, 2023 10:29
@mdsteele
Copy link
Owner

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.)

This was referenced Mar 25, 2023
@ikrivosheev
Copy link
Contributor Author

ikrivosheev commented Mar 25, 2023

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

  1. feat: Upgrade edition and examples #18
  2. Replace deprecated actions-rs to dtolnay/rust-toolchain #17
  3. feat: Less allocations #19

After I will continue splitting this PR.

@ikrivosheev ikrivosheev deleted the feature/rewrite_api branch March 29, 2023 08:40
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants