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

Don't use exec() for file #42

Merged
merged 20 commits into from
Mar 12, 2022
Merged

Conversation

erkyrath
Copy link
Member

@erkyrath erkyrath commented Mar 7, 2022

Note: This PR is built on top of #41 ! Merge that first.

I've written a little utility to pipe the output of unzip/untar into file.

This uses the Node event-driven file-stream API. All the output of unzip -p xxx.zip file goes through the unbox process. But we don't store it all, not even in memory. It's just passed along chunk by chunk.

(The stdout/stderr output of file does get accumulated in memory. But that's tiny.)

Now, the bad news: this doesn't fix the problems that I originally thought it would fix. In #39 , I mentioned two files causing errors:

Turns out these are not shell bugs. They are mismatches between what unzip reports in its file list and what it accepts as a filename. Oh well!

(I recognize the Icon^M thing. It's a MacOS Classic convention where a folder icon appeared in the filesystem with that filename, yes, with a ctrl-M in it. Everybody hated that.)

Anyhow, this spurred me into cleaning up the exec() situation, and I'm very happy about that. I think my implementation is correct. It's possible I missed something; I still haven't spent a lot of time in async-node-land.

@erkyrath
Copy link
Member Author

erkyrath commented Mar 7, 2022

I see the lint error but I don't know what the idiom is to make it go away.

@curiousdannii
Copy link
Member

Arrow functions with no arguments have the syntax of () => {}. Or function() {} works too ;).

@erkyrath
Copy link
Member Author

erkyrath commented Mar 7, 2022

Thanks -- I knew it had to be something obvious.

@curiousdannii curiousdannii merged commit 0d93d71 into iftechfoundation:master Mar 12, 2022
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