mongodb-js / mj

#wip mongodb-js tooling
https://github.com/mongodb-js/mj
Apache License 2.0
0 stars 2 forks source link

first stab at `mj install` #12

Closed rueckstiess closed 9 years ago

rueckstiess commented 9 years ago
current progress

mj install will

and additionally, if --sublime supplied, search for sublime installation to

Review on Reviewable

imlucas commented 9 years ago

Comments from the review on Reviewable.io

Reviewed files:


commands/install.js, line 132 [r1] (raw file): If already installed, npm install will just be a noop so don't worry


package.json, line 18 [r1] (raw file): Why all of lodash when you're just using amp-each and amp-has?


test/index.test.js, line 95 [r1] (raw file): Use if(err) return cb(err); or assert.ifError(err); instead


test/index.test.js, line 106 [r1] (raw file): Use if(err) return cb(err); or assert.ifError(err); instead


util/run_steps.js, line 9 [r1] (raw file): If we're going to use jsdocs, let's use them everywhere and format properly https://www.npmjs.com/package/dox


imlucas commented 9 years ago

Comments from the review on Reviewable.io

Reviewed files:


imlucas commented 9 years ago

Comments from the review on Reviewable.io


Looking good. Just a few questions I'm curious about.


rueckstiess commented 9 years ago

Comments from the review on Reviewable.io


commands/install.js, line 132 [r1] (raw file): I know but it takes 3-5 seconds. Would be nice if it was instant.


package.json, line 18 [r1] (raw file): good point, will replace with amp-*.


imlucas commented 9 years ago

Comments from the review on Reviewable.io


commands/install.js, line 132 [r1] (raw file): Ah I see. Comment made me think you needed for sanity. This will cut it down to ~500ms:

var exec = require('child_process').exec;

/**
 * Check if a module with `name` is installed globally.
 *
 * @param {String} name of the module, e.g. jshint
 * @param {Function} fn (error, exists)
 */
function isModuleInstalledGlobally(name, fn){
  var cmd = 'npm ls --global --production --json --depth=0';
  exec(cmd, function(err, stdout){
    if(err) return fn(err);

    var data = JSON.parse(stdout);
    var installed = data.dependencies[name];
    fn(null, (installed !== undefined));
  });
}
// npm install pretty-hrtime
var prettyHrtime = require('pretty-hrtime');
var start = process.hrtime();
isModuleInstalledGlobally('jshint', function(err, installed){
  var duration = process.hrtime(start);
  console.log('exec of npm ls took', prettyHrtime(duration));

  if(err) return console.error(err);
  console.log('jshint installed globally?', installed);
});

imlucas commented 9 years ago

Comments from the review on Reviewable.io

Reviewed files:


imlucas commented 9 years ago

Comments from the review on Reviewable.io


:thumbsup:


rueckstiess commented 9 years ago

Comments from the review on Reviewable.io


commands/install.js, line 132 [r1] (raw file): Perfect, thanks! --json is great.

Replacing and merging in PR.