Skip to content

Conversation

@AnneKitsune
Copy link

Hello there!

Really cool crate, I like it a lot! I'll probably open more pull requests as I go.

Copy link
Owner

@jaemk jaemk left a comment

Choose a reason for hiding this comment

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

awesome thanks! Just a few small thing

io::copy(&mut file, &mut output)?;
let outpath = into_dir.join(file.sanitized_name());

if (&*file.name()).ends_with('/') {
Copy link
Owner

Choose a reason for hiding this comment

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

can you use outpath.is_dir() instead to be windows compatible

Copy link
Author

Choose a reason for hiding this comment

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

That was copy pasted from zip-rs example, but yes I can.

Copy link
Author

Choose a reason for hiding this comment

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

Doing that causes the program to think everything is a file. I think this is mostly due to zip-rs handling folders as files and only making the difference using the ending /.

Copy link
Owner

Choose a reason for hiding this comment

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

hm, that's unfortunate... Do you know if that's also the case on windows? Could you maybe setup a test zip file with nested dirs and add windows-cfg'd/linux-cfg'd tests that check this?

Copy link
Author

Choose a reason for hiding this comment

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

I think the reason for the is_dir to not work is because of the sanitized_name call, which might be removing the trailing slash.

I'm a bit(very) short on time currently, so I don't want to spend a lot of time on this bug fix writing unit tests. Sorry!

src/lib.rs Outdated
let outpath = into_dir.join(file.sanitized_name());

if (&*file.name()).ends_with('/') {
fs::create_dir_all(&outpath).unwrap();
Copy link
Owner

Choose a reason for hiding this comment

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

can you use the ? operator instead of unwraps

Copy link
Author

Choose a reason for hiding this comment

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

Fixed

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.

2 participants