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

feat: refactor archive type #748

Open
wants to merge 5 commits into
base: main
Choose a base branch
from

Conversation

nichmor
Copy link
Contributor

@nichmor nichmor commented Jun 11, 2024

Based on my previous PR, I thought about making Archive type a building block so that you could reuse where needed.

This is how you would use it in rattler-build:
prefix-dev/rattler-build#922

crates/rattler_package_streaming/src/read.rs Outdated Show resolved Hide resolved
crates/rattler_package_streaming/src/archive.rs Outdated Show resolved Hide resolved
crates/rattler_package_streaming/src/read.rs Outdated Show resolved Hide resolved
@baszalmstra
Copy link
Collaborator

I like it! Maybe we can rename it LocalArchive to indicate that its a file on disk. There is also a lot of //test in the code. Could you clean this up? And add a few tests?

@nichmor
Copy link
Contributor Author

nichmor commented Jun 11, 2024

I like it! Maybe we can rename it LocalArchive to indicate that its a file on disk. There is also a lot of //test in the code. Could you clean this up? And add a few tests?

sure! I've added //test to show how the refactor would look like and to see if the refactor is appropriate one

@nichmor nichmor requested a review from baszalmstra June 12, 2024 14:29
@nichmor
Copy link
Contributor Author

nichmor commented Jun 12, 2024

I like it! Maybe we can rename it LocalArchive to indicate that its a file on disk. There is also a lot of //test in the code. Could you clean this up? And add a few tests?

done!

Copy link
Collaborator

@baszalmstra baszalmstra left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Perhaps you could also add methods for other functions, like extracting the entire archive? Another awesome feature would be to be able to extract anything that implements PackageFile.


impl LocalArchive {
/// Extracts the contents of the archive to the specified destination.
pub fn extract_a_folder(
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Can we also rename this to match the function it calls?

Suggested change
pub fn extract_a_folder(
pub fn extract_directory(

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