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

Add strip option #13

Merged
merged 1 commit into from Feb 28, 2014
Merged

Add strip option #13

merged 1 commit into from Feb 28, 2014

Conversation

kevva
Copy link
Contributor

@kevva kevva commented Oct 11, 2013

No description provided.

@wibblymat
Copy link
Member

I have a couple of problems here. Firstly you can't use a literal / in anything to do with paths or it won't work on Windows. You need to use path.sep.

Also, I'd like for there to be at least a warning if there are fewer path elements than the strip number. If you have a tree that only goes (for e.g.) two deep and have a strip of 4 then all files will end up in the same place. If two files have the same name one will overwrite the others.

At https://github.com/bower/decompress-zip/blob/master/lib/decompress-zip.js#L89 you could loop through the files and check that they all have a common path prefix of the right length and error out before actually doing any extracting. You could transform the paths at that point too.

I think my preference would actually be that instead of saying strip: 2 you should have to say strip: "angular/dist". Then only files that are in that path will be extracted. But that's kind of a different feature so maybe not.

@kevva
Copy link
Contributor Author

kevva commented Oct 12, 2013

Whoops, missed path.sep. I'll fix that and add the path depth check. As for using strip: 'some/path' I'm in doubt. I think it's better to stick to the standard (equivalent to --strip-components). That feature should probably go under a different option.

@kevva
Copy link
Contributor Author

kevva commented Oct 14, 2013

@wibblymat, should it process each file individually and check if options.strip is deeper than file.path? Or maybe you meant that it should check for the deepest path in all files and check it against options.strip?

@sindresorhus
Copy link
Contributor

@wibblymat ⬆️

@wibblymat
Copy link
Member

@kevva I think I meant that you check up front that each file that is going to be extracted (i.e. after the filter has been applied) is at least as deep as options.strip

@kevva
Copy link
Contributor Author

kevva commented Jan 28, 2014

@wibblymat, sorry for taking such time. Should be ok now.

@@ -96,6 +96,21 @@ DecompressZip.prototype.extract = function (options) {
});
}

if (options.strip) {
files = files.filter(function (file) {
Copy link
Member

Choose a reason for hiding this comment

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

I think this is should be map, not filter.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yeah, true. Fixed.

@kevva
Copy link
Contributor Author

kevva commented Feb 4, 2014

ping @wibblymat

wibblymat added a commit that referenced this pull request Feb 28, 2014
@wibblymat wibblymat merged commit 1650e8c into bower:master Feb 28, 2014
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants