comments for project

lib/error-handling.js

@@ -1,3 +1,4 @@
+//I don't see this file being used anywhere, if that's the case I'd just take it out
 module.exports = exports = function(err, req, res, next) {
   res.status(500).json({ msg: err.message });
   next();

lib/init.js

@@ -3,6 +3,11 @@
 const fs = require('fs-extra');
 const questions = require('./init-questions.json');
 
+//If you find yourself nesting callbacks this deep it's a good time
+//to start thinking about handling async some other way. If you were
+//to use promises for instance you could have even taken all of those
+//"if (err)" conditionals to that single catch at the end.
+
 const init = function(args, callback){
   let jsonStuff;
   this.prompt(questions)
@@ -15,6 +20,9 @@ const init = function(args, callback){
         fs.mkdirs('./template', (err)=>{
           if (err) this.log(err);
           this.log('Here is your README.md, plop.json and template directory: \n');
+          //Unless you have a very specific reason that you need to use sync I'd
+          //use async everywhere you can and I think you could have here. If you're
+          //worried about nesting try something else for async.
           let newPlop = fs.readdirSync('./');
           for (var i = 0; i < newPlop.length; i++){
             this.log(newPlop[i]);
@@ -24,6 +32,9 @@ const init = function(args, callback){
       });
     });
   }).catch(err => this.log(err));
+  //I'm guessing that this is something that vorpal needs you to pass in so since it's
+  //waiting on a prompt it's probably not too important, but calling this callback here
+  //wouldn't wait on your async calls.
   callback();
 };
 

lib/install.js

@@ -19,6 +19,7 @@ const install = function(args, callback){
       return err;
     });
   } else {
+    //Break strings this long down to multiple lines, you can do so by concatenating.
     this.log(new Error(`Failed to install plop template because ~/.config/plop/${rename} already exists.\nPlease try again with different name.`));
   }
   callback();

test/list-test.js

@@ -2,14 +2,14 @@
 
 const chai = require('chai');
 const expect = chai.expect;
-
+const fs = require('fs-extra');
 const list = require(__dirname + '/../lib/list');
 
 describe('should test the list functionality', () => {
   it('should identify list as a function', () => {
     expect(list).to.eql.function;
   });
-
+  //this test doesn't test anything.
   it('should identify list type', () => {
     let files = [];
     for(var i = 0; i < files.length; i++) {
@@ -18,4 +18,60 @@ describe('should test the list functionality', () => {
       expect(files).to.gte(0);
     }
   });
+
+  //Here's an example of a way you could have tested your list function:
+
+  // let backUpHome;
+  // before((done) => {
+  //   //backup your HOME property so you don't mess up other tests.
+  //   backUpHome = process.env.HOME;
+  //   //set home to __dirname which should be the test directory
+  //   process.env.HOME = __dirname;
+  //   //create the directory that list expects to see
+  //   fs.mkdirs(__dirname + '/.config/plop', err => {
+  //     if (err) throw err;
+  //     //put a file there for list to pick up
+  //     fs.writeFile(__dirname + '/.config/plop/test.txt','', err => {
+  //       if (err) throw err;
+  //       done();
+  //     });
+  //   });
+  // });
+
+  // after((done) => {
+  //   //reset HOME
+  //   process.env.HOME = backUpHome;
+  //   //clean up your mock .config
+  //   fs.remove(__dirname + '/.config', err => {
+  //     if (err) throw err;
+  //     done();
+  //   });
+  // });
+
+  // it('should log files', (done) => {
+  //   //The toughest part of your list function as it stands is that it refers
+  //   //to something on 'this' remember that context is a moving target
+  //   //and you can tell it to point somewhere else just like anything
+  //   //else.
+
+  //   //make an object with a log function that checks what's being passed in,
+  //   //tests it, and calls done.
+  //   let mockThis =  {
+  //     log: function(args) {
+  //       expect(args).to.eql('test.txt');
+  //       done();
+  //     }
+  //   };
+
+  //   //Here's the tricky part:
+  //   //.call is a method on the function prototype that calls a function.
+  //   //the first argument you pass in is what the value of 'this' will be
+  //   //inside that function. That means that whereever your list function
+  //   //calls this.log (lines: 7, 8, and 10) 'this' will be the mockThis
+  //   //object we just created and this.log will call the function we just
+  //   //wrote and pass in the arguments. The other arguments are the arguments
+  //   //that get passed into that function. So args and callback respectively.
+  //   list.call(mockThis, [], function(){});
+  // });
+
 });