bower / decompress-zip

Module that decompresses zip files
MIT License
102 stars 76 forks source link

Added file mode preservation #32

Closed szwacz closed 8 years ago

szwacz commented 10 years ago

Fixes https://github.com/bower/decompress-zip/issues/22

rayshan commented 10 years ago

Thank you for the PR @szwacz, can you please add some tests?

szwacz commented 10 years ago

Well I wanted to, but...

  1. I imagine I'll need prepared zip files for tests, and zips used for current testing are downloaded from third-party server. How can I add more test files then?
  2. Your tests are right now platform-specyfic, and can't run on Windows at all, what I consider to be broken right from the beginning.
szwacz commented 10 years ago

I'd love to add tests, but I'm afraid it requires refactoring on project owners side first, for now adding more tests is a little too hard.

szwacz commented 10 years ago

I would like to see this change in npm shortly, and tests refactoring looks like not that much of work, so I can do it, but tell me if you agree to move test zip files from https://drive.google.com/uc?id=0Bxxp2pVhWG1DTFNWQ1hsSkZKZmM&export=download into this repository (and it will make more sense to filter them with .npmignore, so tests will be runnable only by fetching the git repo). Sounds good?

sindresorhus commented 10 years ago

@rayshan :arrow_up:

rayshan commented 10 years ago

Sorry I drop the ball on this one. I'm back from vacation now.

@szwacz are you referring to the test assets in bower repo here? https://github.com/bower/bower/tree/master/test/assets

I agree it makes sense for decompress assets and tests to live in this module, however I'm not as familiar with bower CLI architecture and this module. @sheerun or @sindresorhus can you please advise?

szwacz commented 10 years ago

@rayshan Decompress-zip project downloads test assets from https://drive.google.com/uc?id=0Bxxp2pVhWG1DTFNWQ1hsSkZKZmM&export=download (see download-test-assets.js). Those assets looks totally different than that mentioned by you from bower repository.

rayshan commented 10 years ago

I see, never mind about what I said. Yes I agree test fixtures should live in this repo just like bower repo's test fixtures. Perhaps the original concern was that the file was too big? If we don't need all the packages in the asset archive, maybe we can remove /angular, which is the bulk of the size.

Also agree on the cross-platform concern, since the team supports the CLI on Windows.

sheerun commented 10 years ago

Please make zip files as small as possible when moving :)

sindresorhus commented 9 years ago

Merge conflicts needs to be resolved ;)

szwacz commented 9 years ago

Yey! Ok, I'll fix this and add some tests tomorrow or on Tuesday. Please stay tuned ;)

sheerun commented 9 years ago

@szwacz I've merged your PR with tests refactor to master and even added coveralls support :)

coveralls commented 9 years ago

Coverage Status

Coverage increased (+0.2%) when pulling 0d1c79cda6686242739a01a8a3b9ed78a2aa3fd1 on szwacz:file-mode-preservation into 55ac1e7e5fd98f00176c4951a515bfdc417280fd on bower:master.

coveralls commented 9 years ago

Coverage Status

Coverage decreased (-0.72%) when pulling 63663add837564128306fc0f9a725c9e47a24c16 on szwacz:file-mode-preservation into 55ac1e7e5fd98f00176c4951a515bfdc417280fd on bower:master.

szwacz commented 9 years ago

All right. Here are unit tests.

szwacz commented 9 years ago

One more thing... My code doesn't cover case when you extract stuff into nonexistent folder, e.g.

zip.extract({ path: 'completely/new/path' });

Folders completely/new/path don't exist, so DecompressZip will create them, but there is no direct way to set their mode. Of course we can do something like:

zip.extract({ path: 'completely/new/path', modeForNonexistentDirectories: '777' });

But I don't know if this feature will be used at all. What do you think?

szwacz commented 9 years ago

Ok, ignore what I've written one comment above. If someone wants to have folders with certain mode he or she could create them bofore unpacking or chmod them after unpacking. It doesn't make sense to add this to DecompressZip.

szwacz commented 9 years ago

Ping?

iwege commented 9 years ago

+1 for this PR

brian-mann commented 9 years ago

+1 could use this!