Skip to content
This repository was archived by the owner on Dec 15, 2022. It is now read-only.

Support bzip2#14

Merged
darangi merged 1 commit intoatom:masterfrom
YtvwlD:bzip2
Apr 16, 2020
Merged

Support bzip2#14
darangi merged 1 commit intoatom:masterfrom
YtvwlD:bzip2

Conversation

@YtvwlD
Copy link
Copy Markdown
Contributor

@YtvwlD YtvwlD commented Oct 27, 2019

Issue or RFC Endorsed by Atom's Maintainers

This is mostly an effort to implement atom/archive-view#31, but may be useful for other packages outside of Atom.

Description of the Change

This adds functionality to open bzip2 files and bzip2 wrapped tar archives.
I mostly copied the code and tests for gzip files.
The actual decompression of bzip2 is done by an added dependency.

Alternate Designs

There's a big amount of duplicate code on this package.
Instead of reorganizing stuff, I tried to add this feature while keeping the diff small.

Possible Drawbacks

Opening bzip2 files may be slower than gzip files, because the compression is slower and because it is being done by a JavaScript library instead of Node itself.

Verification Process

There are tests, but just try to open a .tar.bz2 file, list its contents and extract a file.

Release Notes

Supports to open bzip2 files and bzip2 wrapped tar archives now.

@YtvwlD
Copy link
Copy Markdown
Contributor Author

YtvwlD commented Oct 27, 2019

I don't think the CI failure is my fault. Travis runs this with node 0.10 - the sub-dependency which is failing (https://github.com/atom/fs-plus) only supports node 6 and higher.

@YtvwlD
Copy link
Copy Markdown
Contributor Author

YtvwlD commented Jan 12, 2020

Should I open a new pull request to bump the Node version Travis uses? (Otherwise, I don't think there's a chance the CI pipeline will succeed.)

@lkashef
Copy link
Copy Markdown

lkashef commented Jan 17, 2020

@YtvwlD, I confirmed that your PR passes the checks using node 6.

What you are suggesting would be ideal, ping me once you have the PR ready and I will merge it in.

@YtvwlD
Copy link
Copy Markdown
Contributor Author

YtvwlD commented Jan 22, 2020

I rebased this PR onto current master. This should make the CI succeed.

@darangi darangi self-assigned this Apr 3, 2020
@darangi
Copy link
Copy Markdown
Contributor

darangi commented Apr 7, 2020

Hi @YtvwlD I am currently taking a look at this PR, awesome stuff! I am trying to get this to work locally, I did some linking from node-ls-archive -> archive-view -> atom, I verified the linking works locally but the PR isn't behaving as expected, I tried un-archiving the .tar.bz2 file in the fixtures but it did not work as expected, any pointers on how you verified that it works locally?

@YtvwlD
Copy link
Copy Markdown
Contributor Author

YtvwlD commented Apr 7, 2020

I'm not quite sure I understand what you mean.

I could extract .tar.bz2 in the fixtures with GNU tar and I did run the tests I wrote locally before submitting the PR. They also run fine in Travis.

There's no integration in archive-view yet, AFAIK, so I didn't test this in Atom.

Running this on the command line works for me:

~/dev/node-ls-archive/bin$ ./lsa ../spec/fixtures/one-file.tar.bz2 
/home/niklas/dev/node-ls-archive/spec/fixtures/one-file.tar.bz2 (1)
└── file.txt

@darangi
Copy link
Copy Markdown
Contributor

darangi commented Apr 7, 2020

Thanks @YtvwlD, I thought you tested it on atom. I get it now 👍

@darangi darangi merged commit e5e9e6d into atom:master Apr 16, 2020
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

5 participants