-
Notifications
You must be signed in to change notification settings - Fork 83
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
Create new submodule update target to run git submodule update #61
base: master
Are you sure you want to change the base?
Changes from 6 commits
928a463
1824a5c
2e205ac
4b5f632
0f0b8f6
e3380fd
8984b98
4f22223
384bfaf
1278809
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,56 @@ | ||
'use strict'; | ||
|
||
var async = require('grunt').util.async; | ||
var grunt = require('grunt'); | ||
var _s = require('underscore.string'); | ||
|
||
module.exports = function (task, exec, done) { | ||
var optionKey; | ||
var allowedOptions = { | ||
init: false, | ||
remote: false, | ||
force: false, | ||
rebase: false, | ||
merge: false, | ||
reference: null, | ||
recursive: false, | ||
noFetch: false | ||
}; | ||
|
||
var options = task.options(allowedOptions); | ||
|
||
var args = ['submodule', 'update']; | ||
|
||
|
||
// Loop through allowable cli flags in options and add to args | ||
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. This is where I feel like I've done things differently than you would have, @rubenv ;-) If you prefer, I am happy to change this to explicitly pushing individual args onto the args array, rather than doing so in a loop. Notice that we are looping through 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. No it's cool, it cuts back on the amount of lines. Might be a good future improvement to convert all binary options everywhere to a logic similar to this. We'd need to handle dashes: |
||
for (optionKey in allowedOptions) { | ||
if (options.hasOwnProperty(optionKey) && options[optionKey]) { | ||
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.
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. Agreed. I had to do a little research to confirm, but it looks like my understanding of property enumeration was very outdated. After ES5, it is quite simple to mark prototype properties as non-enumerable, so adding https://groups.google.com/forum/#!topic/jsmentors/2kmsNxOirFk |
||
// Add flag | ||
args.push('--' + _s.dasherize(optionKey)); | ||
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. dasherize uses the following regex: return _s.trim(str).replace(/([A-Z])/g, '-$1').replace(/[-_\s]+/g, '-').toLowerCase(); |
||
//args.push('--' + optionKey); | ||
// If not a boolean, add the value after the flag | ||
if (typeof options[optionKey] !== 'boolean') { | ||
args.push(options[optionKey]); | ||
} | ||
} | ||
} | ||
|
||
// If a path was specified, add it now: | ||
if (options.path) { | ||
args.push(options.path); | ||
} | ||
|
||
// Depth also needs to be specified explicitly here | ||
// because a depth of zero would be skipped in the loop | ||
if (options.depth !== null && options.depth !== undefined) { | ||
args.push('--depth'); | ||
args.push(options.depth); | ||
} | ||
|
||
// Add callback | ||
args.push(done); | ||
|
||
exec.apply(this, args); | ||
}; | ||
|
||
module.exports.description = 'Update git submodules.'; |
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -42,5 +42,8 @@ | |
}, | ||
"keywords": [ | ||
"gruntplugin" | ||
] | ||
], | ||
"dependencies": { | ||
"underscore.string": "~2.3.3" | ||
} | ||
} |
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,115 @@ | ||
'use strict'; | ||
|
||
var command = require('../lib/commands').submoduleupdate; | ||
var Test = require('./_common'); | ||
|
||
describe('submodule update', function () { | ||
it('should update submodules', function (done) { | ||
var options = { | ||
}; | ||
|
||
new Test(command, options) | ||
.expect(['submodule', 'update']) | ||
.run(done); | ||
}); | ||
|
||
it('should accept init option', function (done) { | ||
var options = { | ||
init: true | ||
}; | ||
|
||
new Test(command, options) | ||
.expect(['submodule', 'update', '--init']) | ||
.run(done); | ||
}); | ||
|
||
it('should accept remote option', function (done) { | ||
var options = { | ||
remote: true | ||
}; | ||
|
||
new Test(command, options) | ||
.expect(['submodule', 'update', '--remote']) | ||
.run(done); | ||
}); | ||
|
||
it('should accept no-fetch option', function (done) { | ||
var options = { | ||
noFetch: true | ||
}; | ||
|
||
new Test(command, options) | ||
.expect(['submodule', 'update', '--no-fetch']) | ||
.run(done); | ||
}); | ||
|
||
it('should accept force option', function (done) { | ||
var options = { | ||
force: true | ||
}; | ||
|
||
new Test(command, options) | ||
.expect(['submodule', 'update', '--force']) | ||
.run(done); | ||
}); | ||
|
||
it('should accept rebase option', function (done) { | ||
var options = { | ||
rebase: true | ||
}; | ||
|
||
new Test(command, options) | ||
.expect(['submodule', 'update', '--rebase']) | ||
.run(done); | ||
}); | ||
|
||
it('should accept merge option', function (done) { | ||
var options = { | ||
merge: true | ||
}; | ||
|
||
new Test(command, options) | ||
.expect(['submodule', 'update', '--merge']) | ||
.run(done); | ||
}); | ||
|
||
it('should accept reference option', function (done) { | ||
var options = { | ||
reference: 'https://myrepo.com/repo.git' | ||
}; | ||
|
||
new Test(command, options) | ||
.expect(['submodule', 'update', '--reference', 'https://myrepo.com/repo.git']) | ||
.run(done); | ||
}); | ||
|
||
it('should accept depth option', function (done) { | ||
var options = { | ||
depth: 10 | ||
}; | ||
|
||
new Test(command, options) | ||
.expect(['submodule', 'update', '--depth', '10']) | ||
.run(done); | ||
}); | ||
|
||
it('should accept recursive option', function (done) { | ||
var options = { | ||
recursive: true | ||
}; | ||
|
||
new Test(command, options) | ||
.expect(['submodule', 'update', '--recursive']) | ||
.run(done); | ||
}); | ||
|
||
it('should accept path option', function (done) { | ||
var options = { | ||
path: '/test/path' | ||
}; | ||
|
||
new Test(command, options) | ||
.expect(['submodule', 'update', '/test/path']) | ||
.run(done); | ||
}); | ||
}); |
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.
Using the underscore string library seems like a more maintainable solution than inserting our own regex into each of the
lib/command_*
files. Underscore.string provides a method calleddasherize
that converts cammelCase strings to dash-separated strings. https://github.com/epeli/underscore.string.Here is the regex that they use:
This will convert
noFetch
tono-fetch
, but would convertNoFetch
to-no-fetch
.An alternative approach would be to create our own
lib/util.js
that contains a method to convert camelCase to dashes. From what I've read, the most efficient algorithm would be the following:See http://jsperf.com/js-camelcase for a comparison of regex performance going from a space-delimited string to camelCase. Using a sigle regex with a callback function was about 20% more efficient than chaining two replace methods.
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.
Note: the jshint test now fails due to the variable starting with an underscore. Would you prefer to modify the jshint config, or modify the variable name?
_s
is the standard name for the underscore.string utility.