diff --git a/lib/error-handling.js b/lib/error-handling.js index 264dae4..b469f80 100644 --- a/lib/error-handling.js +++ b/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(); diff --git a/lib/init.js b/lib/init.js index fda7d87..e78f2bc 100644 --- a/lib/init.js +++ b/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(); }; diff --git a/lib/install.js b/lib/install.js index 76a4816..e7d424a 100644 --- a/lib/install.js +++ b/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(); diff --git a/test/list-test.js b/test/list-test.js index 7ea54d3..382fe5b 100644 --- a/test/list-test.js +++ b/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(){}); + // }); + });