Skip to content
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

Added filter option to allow filtering of submodules using config. #1

Merged
merged 3 commits into from
Nov 4, 2014

Conversation

benjamind
Copy link
Contributor

I really needed to be able to filter out some submodules which don't support Grunt tasks. This gives you an option 'filter' which you can specify in the task configuration. It specifies a standard minimatch filter string.

@jpommerening
Copy link
Owner

I like the idea!

Give me some time to fix the Travis issue so you can rebase onto a working commit.

In the meantime, how about making that options.filter instead of data.filter?
Maybe, to avoid the chain of ternary operators, we could try this:

var options = this.options({
  base: '.',
  gruntfile: 'Gruntfile.js',
  filter: arguments[0],
  tasks: /* unchanged */
});
var data = /* unchanged */;
var done = /* unchanged */;
var filter = options.filter ? minimatch.filter(options.filter) : function () { return true; };

@benjamind
Copy link
Contributor Author

That sounds sensible.

Not sure we can just set filter to arguments[0], that would change the current default behaviour for the CLI wouldn't it?

… { filter: 'filter string' } } now. Also modified way grunt fork is called so that we correctly use subdirectory as working directory since prior method caused some modules to build incorrectly.
@benjamind
Copy link
Contributor Author

OK I've modified this to now use the 'options' property instead. I removed the arguments[0] reference since I think it changes the CLI interface and we don't want to create breaking changes.

I also fixed a small bug in the way the grunt fork is called.

Previously we executed grunt from the current working directory but passed in the submodule folder for it to work on. This caused any grunt scripts that depended on current working directory paths to fail. Fork now uses 'cwd' property to set the current working directory correctly.

@jpommerening
Copy link
Owner

Not sure we can just set filter to arguments[0], that would change the current default behaviour for the CLI wouldn't it?

That is the current default behaviour of the CLI. You can filter by the first parameter, as in: grunt submodule:deps/* (were you referring to something else?). The object passed to this.options() contains default values which can be overridden inside your Gruntfile.

I'm not so sure about the current working directory. I thought tasks/lib/grunt.js should already handle that just fine, by calling process.chdir just before starting up. Anyway, it seems to work for you so I might be wrong.

Sorry this is taking so long, I'm having some trouble (and a lack of patience) with the tests :(

@jpommerening jpommerening merged commit c7edd1a into jpommerening:master Nov 4, 2014
@jpommerening
Copy link
Owner

Merged it. I think I'll write some more tests and npm publish later.

Can you try the current master and see if it works as expected?

Thanks!

Edit: try npm install grunt-submodule@0.1.0-rc2
Cheers!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants