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

Section3/Storage/lib/data.js #1

Open
RashikAnsar opened this issue Apr 2, 2018 · 4 comments
Open

Section3/Storage/lib/data.js #1

RashikAnsar opened this issue Apr 2, 2018 · 4 comments

Comments

@RashikAnsar
Copy link

RashikAnsar commented Apr 2, 2018

// Delete a file
lib.delete = function(dir,file,callback){
  // Unlink the file
  fs.unlink(lib.baseDir+dir+'/'+file+'.json', function(err){
    if(!err){
      callback(false);
    }else{
      callback(err);
    }
  });
};

delete file code has to be updated

@pirple-author
Copy link
Contributor

Nice work Rashik, thank you. Unfortunately I need to update this library in every data.js file in every lecture, in every section of this repo. So this may take a few days, but I'll do it as fast as I can. Thank you for bringing this to my attention!

@pirple-author
Copy link
Contributor

(Leaving this issue open to help me remember to do it)

@gabeno
Copy link

gabeno commented Apr 25, 2018

A small refactor: making use of javascript template strings

`${lib.baseDir}${dir}/${file}.json`

Further, regarding:

Unfortunately I need to update this library in every data.js file in every lecture, in every section of this repo.

I am curious if we can do PRs. It will obviously be faster and you can still keep an eye on things as you review merging.

@pirple-author
Copy link
Contributor

Hi @gabeno, that's a great change for you to make in your local repo.

But unfortunately, there's two issues that would stop me from making that change:

  1. ES6/ES2015 is not a prerequisite for this course (students are not required to know it before taking this course), so I've adamantly avoided all traces of it (thus no promises, arrows, template literals etc). It was a complicated decision, but in order to make this course accessible to the widest possible audience, I had to leave ES6 out.

  2. Even if that change was doable, it doesn't match what I create in the video. Rashik's pull request above is actually changing the repo to align better with the video, not diverting it away from it, which is why I'm happy to make that change. But I've got to keep the code here lined up with the video or it will confuse future students.

Thanks for your attention to the issue though. And to answer your question, yes generally PRs are fine. They've just got to be complete enough that all the lectures are updated. Otherwise it'll go to the bottom of my very-long todo list, as this one did.

FlyMaple pushed a commit to FlyMaple/The-NodeJS-Master-Class that referenced this issue Oct 29, 2018
Signed-off-by: Skye.Wu <[email protected]>
rachmann pushed a commit to rachmann/The-NodeJS-Master-Class that referenced this issue Dec 8, 2020
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

No branches or pull requests

3 participants