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

Adding in support for fs.copyFile. #436

Open
Mordax opened this Issue Sep 21, 2018 · 8 comments

Comments

Projects
None yet
3 participants
@Mordax

Mordax commented Sep 21, 2018

I noticed that Filer does not support fs.copyFile. What's your opinion on me taking on writing out the code/doing a test for this? Difficult or good feature to work on? @humphd

@humphd

This comment has been minimized.

Contributor

humphd commented Sep 21, 2018

Nice catch, I didn't realize node had added this to fs. Yes, I think it would be good to have it. Why don't you start by writing a test and submit that, then seen where you're at. You could always use the full implementation as another release later in the course if you find it's too big.

In the simplest case, a copyFile is basically:

function copyFile(src, dest, flags, callback) {
  // Deal with flags, which could be something we add in future PRs...

  fs.readFile(src, null, (err) => {
    if(err) {
      return callback(err);
    }
    fs.writeFile(dest, null, callback);
  });
}
@Mordax

This comment has been minimized.

Mordax commented Sep 21, 2018

Alright, I'll take a crack at it, if you don't mind.

@YuechengWu

This comment has been minimized.

YuechengWu commented Sep 23, 2018

Hi, how is it going with the test, anything you would like to work on together?

@humphd

This comment has been minimized.

Contributor

humphd commented Sep 23, 2018

@YuechengWu I'm not sure if @Mordax is doing just the implementation or if she is doing tests too. Would be great if you two could collaborate somehow.

@Mordax

This comment has been minimized.

Mordax commented Sep 23, 2018

@humphd I took your advice and I'm currently working on the tests first and implementation later. Besides the regular testing, copyFile has some flags that we can test out, @YuechengWu. So there is some more work there.

@humphd

This comment has been minimized.

Contributor

humphd commented Sep 23, 2018

Agree, doing tests for all those various flags, even if we don't land an implementation for them all, would be good.

@YuechengWu

This comment has been minimized.

YuechengWu commented Sep 23, 2018

@Mordax Hi Mordax, what is your name on slack? It would be easier for us to communicate that way if you want.

@Mordax

This comment has been minimized.

Mordax commented Sep 23, 2018

@YuechengWu Sent you a message ✌️

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment