Create new submodule update target to run git submodule update #61

dylancwood · 2014-06-22T04:36:03Z

Resolves #59 at least partially. I have no need for the other submodule commands, but can start adding them if you would like.

My current project needed to perform submodule updates during the build process, so I added this functionality into grunt-git. I tried to maintain the current style, but did do one thing very differently: Instead of pushing options onto the args array individually, I do so in a loop that loops through an array of acceptableOptions. This way, only options that we are expecting will be passed to the child process, and those that might cause an error (e.g. verbose) will not be.

I'm happy to convert it to the more explicit method used elsewhere in this repo if you would prefer.

All CLI flags for git-submodule-update are supported by this patch, though I suspect that most folks will only use init and recursive.

I've also updated the README to include documentation for the new task, and added tests for each option.

dylancwood · 2014-06-22T04:38:37Z

lib/command_submodule_update.js

+    var args = ['submodule', 'update'];
+
+
+    // Loop through allowable cli flags in options and add to args


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 allowedOptions, not options. This will ensure that only the supported options are added to the args array.

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: no-merge -> noMerge.

rubenv · 2014-06-23T12:57:40Z

lib/command_submodule_update.js

+
+    // Loop through allowable cli flags in options and add to args
+    for (optionKey in allowedOptions) {
+        if (options.hasOwnProperty(optionKey) && options[optionKey]) {


hasOwnProperty probably isn't needed. Unless I'm mistaken, the result from options() is a plain object anyway.

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 hasOwnProperty to every for-in loop is no longer necessary.

https://groups.google.com/forum/#!topic/jsmentors/2kmsNxOirFk

dylancwood · 2014-06-23T16:42:38Z

lib/command_submodule_update.js

+
+var async = require('grunt').util.async;
+var grunt = require('grunt');
+var _s = require('underscore.string');


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 called dasherize that converts cammelCase strings to dash-separated strings. https://github.com/epeli/underscore.string.

Here is the regex that they use:

return _s.trim(str).replace(/([A-Z])/g, '-$1').replace(/[-_\s] /g, '-').toLowerCase();

This will convert noFetch to no-fetch, but would convert NoFetch 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:

replace(/([a-z\d][A-Z])/g, function (g) { return g[0] '-' g[1].toLowerCase() });

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.

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.

dylancwood · 2014-06-23T16:44:03Z

lib/command_submodule_update.js

+    for (optionKey in allowedOptions) {
+        if (options.hasOwnProperty(optionKey) && options[optionKey]) {
+            // Add flag
+            args.push('--'   _s.dasherize(optionKey));


dasherize uses the following regex:

return _s.trim(str).replace(/([A-Z])/g, '-$1').replace(/[-_\s] /g, '-').toLowerCase();

dylancwood · 2014-06-23T18:34:39Z

Sorry for all the commits and comments. Here's a concise account:

No need for hasOwnProperty. Fixed
Add string conversion of camelCase options to dash-separated arguments inside of argument generation loop. I elected to use the underscore.string library for this, but am willing to build a separate lib/util.js library if you prefer.
The standard variable name for the underscore.string library is _s, but that name breaks the jshint test, so I am calling it stringUtil.
I modified filtering of options so that options with a value === 0 will be allowed to become args.

Note: I spot-checked a few of the other tasks for options that cannot be converted to CLI flags using underscore's dasherize(). The option noff in gitmerge will not work with the camelCase conversion, since it needs to be converted to --no-ff. There may be others as well.

dylancwood · 2014-06-26T20:47:34Z

Bump. Just checking in on this: no pressure, but wanted to make sure that you weren't waiting on me.

rubenv · 2014-06-27T06:08:55Z

Haven't had time to look into this, but it's on my list! Apologies!
On Jun 26, 2014 10:47 PM, "Dylan Wood" [email protected] wrote:

Bump. Just checking in on this: no pressure, but wanted to make sure that
you weren't waiting on me.

—
Reply to this email directly or view it on GitHub
#61 (comment).

dylancwood · 2014-06-27T15:19:49Z

No problem. Thanks for the update.

rubenv · 2014-06-30T08:31:25Z

I elected to use the underscore.string library for this, but am willing to build a separate lib/util.js library if you prefer.

Nah, just leave it, it's a small dependency anyway.

rubenv · 2014-06-30T08:32:57Z

As it stands, I see no reason not to merge this. Do you reckon we could do the same for other commands?

Note that I'm trying to map tasks to commands 1:1 (see #44). I'm planning to break compatibility in the next minor version to allow that. So don't worry about breaking existing things, there's currently a bugfix 0.2 branch and master holds the future.

dylancwood · 2014-06-30T19:39:28Z

Sounds good. I will apply the same logic to the other tasks, and will look for any task or property names that do not match exactly with the git CLI API. This is going to be a busy week for me, so I expect to tackle this next week.

My plan for the moment is to modify tasks/git.js and add a method for converting options to arguments called optsToArgs(options, optionsForGit, optionsWithoutFlags) that takes the current tasks options, and an array of the options that should be converted and passed to the child process as CLI arguments. It will return an array of dash-separated strings.

Here's a rough implementation:

optsToArgs(options, optionsForGit, optionsWithoutFlags) {
    var args = [];
    // Add 
    for (optionKey in optionsForGit) {
        if (options[optionKey] || options[optionKey] === 0 ) { \\allow numeric zero to pass through        
            // Add flag
            args.push('--'   stringUtil.dasherize(optionKey));                                                         
            // If not a boolean, add the value after the flag                                                          
            if (typeof options[optionKey] !== 'boolean') {                                                             
                args.push(options[optionKey]);
            }
        }       
    }
    // Add arguments that do not have flags (e.g. *repository* arg in git clone)
    for (optionKey in optionsWithoutFlags) {
        if (options[optionKey] || options[optionKey] === 0 ) { \\allow numeric zero to pass through        
            // Add value                                      
            args.push(options[optionKey]);
        }
    }        
    return args;
}

I might come up with a cleaner implementation before I get started though.

gabema · 2015-09-21T22:29:59Z

It would be nice to have support for the git submodule update command via grunt-git. I've merged this branch into my own fork of grunt-git and combined it with some other needed commands and it's been working great.

dylancwood added 3 commits June 21, 2014 21:01

added testing for submodule update

928a463

added submodule_update documentation to README

1824a5c

corrected error about submodule update in readme

2e205ac

dylancwood reviewed Jun 22, 2014
View reviewed changes

allow options.depth to be zero

4b5f632

rubenv reviewed Jun 23, 2014
View reviewed changes

dylancwood added 2 commits June 23, 2014 09:00

added underscore.string to dependencies

0f0b8f6

dasherize cammel-cased arguments

e3380fd

dylancwood reviewed Jun 23, 2014
View reviewed changes

removed commented code

8984b98

dylancwood reviewed Jun 23, 2014
View reviewed changes

dylancwood added 3 commits June 23, 2014 09:56

removed hasOwnProperty and added type checking

4f22223

updated to test depth 0

384bfaf

renamed _.string to pass jshint

1278809

dylancwood mentioned this pull request Jul 13, 2014

Automate conversion of options to arguments #70

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Create new submodule update target to run git submodule update #61

Create new submodule update target to run git submodule update #61

dylancwood commented Jun 22, 2014

dylancwood Jun 22, 2014

rubenv Jun 23, 2014

rubenv Jun 23, 2014

dylancwood Jun 23, 2014

dylancwood Jun 23, 2014

dylancwood Jun 23, 2014

dylancwood Jun 23, 2014

dylancwood commented Jun 23, 2014

dylancwood commented Jun 26, 2014

rubenv commented Jun 27, 2014

dylancwood commented Jun 27, 2014

rubenv commented Jun 30, 2014

rubenv commented Jun 30, 2014

dylancwood commented Jun 30, 2014

gabema commented Sep 21, 2015

		var args = ['submodule', 'update'];


		// Loop through allowable cli flags in options and add to args

Create new submodule update target to run git submodule update #61

Are you sure you want to change the base?

Create new submodule update target to run git submodule update #61

Conversation

dylancwood commented Jun 22, 2014

dylancwood Jun 22, 2014

Choose a reason for hiding this comment

rubenv Jun 23, 2014

Choose a reason for hiding this comment

rubenv Jun 23, 2014

Choose a reason for hiding this comment

dylancwood Jun 23, 2014

Choose a reason for hiding this comment

dylancwood Jun 23, 2014

Choose a reason for hiding this comment

dylancwood Jun 23, 2014

Choose a reason for hiding this comment

dylancwood Jun 23, 2014

Choose a reason for hiding this comment

dylancwood commented Jun 23, 2014

dylancwood commented Jun 26, 2014

rubenv commented Jun 27, 2014

dylancwood commented Jun 27, 2014

rubenv commented Jun 30, 2014

rubenv commented Jun 30, 2014

dylancwood commented Jun 30, 2014

gabema commented Sep 21, 2015