stoeffel / gulp-param

Add params to your tasks
26 stars 8 forks source link

the plugin doesn't work with runSequence #14

Closed przemcio closed 8 years ago

przemcio commented 8 years ago

When I added your plugin into gulpfile I couldn't use runSequence which requires callback argument to be injected via gulp. Here it is an example : gulp.task('default', function (callback) { return runSequence( ['a','b'],callback); } );

after defining gulp as var gulp = require('gulp-param')(require('gulp'), process.argv) I get callback === undefined in default task

stoeffel commented 8 years ago

There is a stale PR to support this. I don't use gulp or this module anymore. Do you want to pick up work on #11? Would be awesome, I currently don't have time to do it. and btw I'm looking for maintainers for this.

przemcio commented 8 years ago

I will look on it :)

2016-01-27 11:30 GMT+01:00 Christoph Hermann notifications@github.com:

There is a stale PR to support this. I don't use gulp or this module anymore. Do you want to pick up work on #11 https://github.com/stoeffel/gulp-param/pull/11? Would be awesome, I currently don't have time to do it. and btw I'm looking for maintainers for this.

— Reply to this email directly or view it on GitHub https://github.com/stoeffel/gulp-param/issues/14#issuecomment-175539090.

stoeffel commented 8 years ago

@przemcio awesome :fireworks:

przemcio commented 8 years ago

I made first clean up in my branch. You may do a peer view if you have a time ;) Generally I would like to implement an approach 1) Match all parameter from args. 2) The first unmatched param will have async callback injected. (async param can have different names not only "callback") 3) There will be an additional parameter in constructor to explicitly set name of async parameter ( 2 point will not be apply then).

stoeffel commented 8 years ago

I made first clean up in my branch. You may do a peer view if you have a time ;)

Sure I will do that.

1) Match all parameter from args.

Jop

2) && 3)

I would prefer just adding the feature where you can explicitly define the name of the async function. IMO less magic is always better.

przemcio commented 8 years ago

I put new version but before I do PR I would like to update README and I have one additional question: In example folder is a gulp file. It works but on the top there is a sample case how to run it // $ gulp test --foo foo --moo moo -b blup I'm wander what for is "-b" ??? There are any parameter which calls "b" but there is "blup" but it is undefined. In my implementation, Setting "blup" is by putting "--blup" as a parameter. Is this OK for you ? Or I've just missed ?

stoeffel commented 8 years ago

put new version but before I do PR I would like to update README

Nice :+1:

I'm wander what for is "-b" ???

here a better example:

// updates the version in `index.js`
gulp.task('updateVersion', function(version) {
    gulp.src(['index.js'])
    .pipe(replace(/version\s=\s\"*.\"/)/, 'version = ' + version))
    .pipe(gulp.dest('index.js'));
});
$ gulp updateVersion --version 0.2.0
# same as
$ gulp updateVersion -v 0.2.0

There should be aliases for each variable.

stoeffel commented 8 years ago

from the readme

It allows the use of shorthand params, but it may cause collisions.

  $ gulp build -d --tag v1.0.0 # -d === --debug
stoeffel commented 8 years ago

We could add an option to disable this feature. What do you think?

przemcio commented 8 years ago

I can add such a mapping but. Is it "a magic" which you mention few msg ago? I wouldn't put this mapping feature at all most due to "it may cause collisions." When you run script you can always rename parameter and argument in function.

stoeffel commented 8 years ago

Agreed. Let's drop it. Simpler is always better. Then we will have to make a major release (it's a breaking change)

przemcio commented 8 years ago

We already did it by introducing a new param in constructor.

Ok, I will prepare a new version of README.md and PR today. What about version should I leave the old one and it will be change when you will publish new version ?

On Fri, Jan 29, 2016 at 11:53 AM Christoph Hermann notifications@github.com wrote:

Agreed. Let's drop it. Simpler is always better. Then we will have to make a major release (it's a breaking change)

— Reply to this email directly or view it on GitHub https://github.com/stoeffel/gulp-param/issues/14#issuecomment-176691952.

stoeffel commented 8 years ago

Awesome! You can leave the version as is.

stoeffel commented 8 years ago

I will publish with npm run major-release which will increase the version number, create a tag and publish to npm.

stoeffel commented 8 years ago

closed with #16