-
-
Notifications
You must be signed in to change notification settings - Fork 298
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
Utility methods to get Git info #314
Changes from 3 commits
a0005df
727073b
312711c
aee42a7
7c24054
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,3 @@ | ||
var shell = require('shelljs'); | ||
|
||
module.exports.shell = shell; | ||
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,28 @@ | ||
// User related helper methods | ||
|
||
var shell = require('./shell').shell; | ||
var _ = require('lodash'); | ||
|
||
|
||
// Exports module | ||
var user = module.exports = {}; | ||
|
||
|
||
// Git related method | ||
// | ||
// The value will come from the global scope or the project scope (it'll take what git | ||
// will use in the current context) | ||
|
||
user.git = {}; | ||
|
||
// Get the current git user.name | ||
// Returns String | ||
user.git.getUsername = function (cb) { | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Can we make these getters instead? Eg: There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I think it should remain a function call so it is obvious that it has a side effect, especially since it can potentially fail. (Edit: typo) There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Same opinion than @passy here. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Although, with a smart cache system that'll take into account CWD context, I think @sindresorhus's right and it'll make for a better API façade - will do! |
||
return shell.exec('git config --get user.name', { silent: true }).output.trim(); | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Since this is a sync slow operation it should be cached so it's only hit one time during the whole lifetime. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Just memoize it There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Yeah, I though about this. The problem is that the username may change depending on where the CWD is. So, we could cache the result related to the CWD, but that seemed like unnecessary optimisation to me. |
||
}; | ||
|
||
// Get the current git user.email | ||
// Returns String | ||
user.git.getEmail = function (cb) { | ||
return shell.exec('git config --get user.email', { silent: true }).output.trim(); | ||
}; |
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,19 @@ | ||
/*global describe, it */ | ||
var assert = require('assert'); | ||
var shelljs = require('shelljs'); | ||
var shellModule = require('../lib/actions/shell'); | ||
var _ = require('lodash'); | ||
|
||
describe('Generator shell methods API', function () { | ||
|
||
it('should be exposed on the Base generator', function () { | ||
assert.equal(shellModule, require('../lib/base').prototype.shell); | ||
}); | ||
|
||
it('should extend shelljs module', function () { | ||
_.each(shelljs, function (method, name) { | ||
assert.equal(method, shellModule.shell[name]); | ||
}); | ||
}); | ||
|
||
}); |
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,39 @@ | ||
/*global describe, before, it, after, before */ | ||
var userUtils = require('../lib/actions/user'); | ||
var shell = require('shelljs'); | ||
var assert = require('assert'); | ||
|
||
describe('user utility', function () { | ||
|
||
it('should be exposed on the Base generator', function () { | ||
assert.equal(userUtils, require('../lib/base').prototype.user); | ||
}); | ||
|
||
describe('git methods', function () { | ||
|
||
before(function () { | ||
this.cwd = process.cwd(); | ||
this.tmp = shell.tempdir(); | ||
shell.cd(this.tmp); | ||
shell.exec('git init --quiet'); | ||
shell.exec('git config --local user.name Yeoman'); | ||
shell.exec('git config --local user.email yo@yeoman.io'); | ||
}); | ||
|
||
after(function () { | ||
shell.cd(this.cwd); | ||
}); | ||
|
||
it('git.getUsername should return the username used by git', function () { | ||
var username = userUtils.git.getUsername(); | ||
assert.equal(username, 'Yeoman'); | ||
}); | ||
|
||
it('git.getEmail should return the email used by git', function () { | ||
var email = userUtils.git.getEmail(); | ||
assert.equal(email, 'yo@yeoman.io'); | ||
}); | ||
|
||
}); | ||
|
||
}); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Why is this a separate file?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Just for concern separation, but I'm open to suggestion.
I hesitated to merge this with
spawn_command
as they're both related to process running in a shell.There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Just require shelljs directly in user.js and base.js?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
That could work. I just based myself on
actions/string.js
to keep consistency. This module does mostly the same thing as I did inactions/shell.js
(less a little logic to merge Lodash and underscore.string).So maybe it'll be best to stick to a style?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Updated for the current. Might be good to consider it for the string module too in the future.