'fail'-reporter fails on errors as well as (mere) warnings

[adam-lynch]

I would've assumed the 'fail' reporter would only fail when there's an error found.

I'm using something like:

.pipe(coffeelint('./coffeelint.json'))
        .pipe(coffeelint.reporter('fail').on('error', function(){
            gutil.beep();
            gulp.run('lint');
        }));

So when it fails, I then run the "default" reporter to show the errors and warnings. When it succeed, nothing is shown.

What I'd like is a parameter like failOnWarning or {failLevel: 'error'} so my .on('error'... is only triggered when an actual error is thrown.

[adam-lynch]

In the meantime, I've writtten gulp-coffeelint-threshold

[janraasch]

To be honest, I quite like your idea. Plus, I think we should really embrace the modularity gulp enables us with. So kudos for creating that sweet gulp-coffeelint-threshold plugin of yours. (Some hints for your gulpplugin: You need to add some basic tests, use through2, etc... Just have a look at the plugin guidelines and acceptance criteria which will hopefully be in place soon.)

Now, on the actual issue: Again, I think you are right. Would be nice to have an option here. Although, I hate having too many options/parameters :) What do you think about just adding a failOnError reporter?

I think that should cover most use cases:

1. Fail on error and warning: Use fail-reporter.
2. Fail only on error: Use failOnError-reporter.

I could publish this as a minor update. So, probably v0.4.0.

Thoughts?

[adam-lynch]

@janraasch thanks.

Yeah, tbh I agree about testing (see my gulp-bless plugin) but I just needed this ready as another project depended on it today.

I did try swapping in through2 but got an error (about an invalid chunk or something like that) so I just went back to through. Will look into it.

I didn't see the acceptance guidelines before, thanks!

---

failOnError sounds good to me. I think there should be an option to determine if the errors and warnings should be printed too though. Right now, the output is shown only for default. It seems strange that these two things are coupled together. If you're using this to enforce a code style like I am, then it would be more likely that you'd want to see the warnings & errors on failure and never on success.

The modularity of Gulp is really great. One downside to my threshold plugin is that you can't turn off the output. Because when using mine after yours, the default reporter has to be used so the actual handling of the failure is delegated to my plugin (unless I guess you could use fail and catch the potential error in .pipe(coffeelint().on('error', gutil.noop)).pipe(threshold(...)) but you can't ask users to manually do that). If you could turn off the output it would aid modularity.

I wonder is there a way we can satisy all use cases with the two plugins. So that you can have linting with output, no output, no failures, fail on error/warning, fail on error only, fail on different thresholds, etc. I like how the gulp-coverage plugin managed to make things more modular.

I guess there might be a way if gulp-coffeelint's main function that just had fail, failOnError, and dontFail (can't think of a better name) modes (with no output) and then the user could optionally pipe the output of that into gulp-coffeelint-threshold then coffeelint.reporter but I can't think of how it could satisfy the case where you might want to throw an error and show the output if a threshold had been exceeded, without showing the output on success.

Modularity is good (the reporter could even be a separate plugin to allow for any order of the three pipes or inserting a custom reporter, etc.) but I don't know if it should cripple gulp-coffeelint into depending on other plugins to work at all.

Any ideas?

[janraasch]

I think I might have misread your issue.

Right now, the output is shown only for default. It seems strange that these two things are coupled together. If you're using this to enforce a code style like I am, then it would be more likely that you'd want to see the warnings & errors on failure and never on success.

»These two things« are actually not coupled at all, I do not think.

• coffeelint() does the actually linting and writes its gatherings to file.coffeelint which it then passes along.
• coffeelint.reporter('default') writes pretty error and warning messages to the console.
• coffeelint.reporter('fail') emits an actually error so that you can fail your build, say on a CI server.

The way I first read your issue was that you would not want coffeelint.reporter('fail') to emit an error on warnings, which is why I said we could add a failOnError reporter.

Now, reading your first comment again, I realize that you seem to be doing a lot of work to achieve something that I think is pretty easy to do. You said, you do something like

.pipe(coffeelint('./coffeelint.json'))
        .pipe(coffeelint.reporter('fail').on('error', function(){
            gutil.beep();
            gulp.run('lint');
        }));

So when it fails, I then run the "default" reporter to show the errors and warnings. When it succeed, nothing is shown.

Actually you could simply run

.pipe(coffeelint())
.pipe(coffeelint.reporter())

to achieve exactly the same behavior, since coffeelint.reporter('default') will pass the file along if file.coffeelint.success === true and will only print anything to the console, if wanrings or errors were found by coffeelint, i.e. »When it succeed, nothing is shown.«.

The only reason coffeelint.reporter('fail') was introduced was so there would be an easy way to fail your build on a CI server. It seems like the readme.md is not very good at explaining this. I should add more details to the Reporters section. For now, you can have a look at gulp-jshint which implements a very similar API.

[janraasch]

I tried to add more details to the readme with this commit https://github.com/janraasch/gulp-coffeelint/commit/1c891c7f552ede5a88d4c8e203346c702f7795b5.

[janraasch]

@adam-lynch, any replies to my last two comments? Otherwise I will close this issue.

[adam-lynch]

Hey, sorry I've been up to walls. I think we still have an issue here. I'll get back to you today/tomorrow :)

[adam-lynch]

So.. I just had a quick look over this again and updated my version of gulp-coffeelint from npm.

Maybe I'm being stupid but you if you have the following, then the errors/warnings aren't shown on failure;

.pipe(coffeelint(paths.other.coffeeRulesFile))
.pipe(coffeelint.reporter('fail'))
.pipe...

I just get:

events.js:72 throw er; // Unhandled 'error' event

Error: CoffeeLint failed for index.coffee --stack trace--

So if you were using fail, you'd have to have the .on('error'... to manually re-run the (default) linting to see the output.

---

By the way, failureOnError would be useful and the readme update is good.

[adam-lynch]

I just noticed this in the readme:

  .pipe(coffeelint())
  .pipe(coffeelint.reporter())
  .pipe(coffeelint.reporter('fail'))

This is close to what I want.

1. It will fail on warnings. So let's assume we're using coffeelint.reporter('failOnError') instead...
2. Then the coffeelint.reporter() step will print warnings even if there's no failure (i.e. no warnings)

[janraasch]

Yes! That is exactly my point. :)

[janraasch]

Here is the code for a failOnError type reporter function:

failOnErrorReporter = ->
    through2.obj (file, enc, cb) ->
        # any errors?
        if file.coffeelint?.errorCount
            @emit 'error', new Error "Found #{file.coffeelint.errorCount} CoffeeLint errors in #{file.relative}"
        @push file
        cb()

[janraasch]

I will leave at that for now. I feel like this is easy enough for people to implement on their own. If more people complain :), we can add this later.

[adam-lynch]

Ok cool. So that's what I meant by "coupled". If you chose to throw errors, you don't see the linting when it fails.

So I guess there's still a need for my plugin. I'll add some tests when I get a chance.

[ejoubaud]

+1 to adding some kind of failOnError reporter, or possibly add thresholds ala @adam-lynch 's plugin as params to the fail reporter. Something like .pipe(coffeelint.reporter('fail', {onWarnings: 0, onErrors: 5})).

What do you think? Happy to submit a PR if you'd like.

[janraasch]

@ejoubaud I would not like to replicate gulp-coffeelint-threshold's functionality here (as I want to keep this as simple and zero-configy as possible), but feel free to fire up a pull request with a failOnError reporter as outlined above :+1:

[adam-lynch]

@ejoubaud Is there something you can't do with gulp-coffeelint + gulp-coffeelint-threshold?

---

feel free to fire up a pull request with a failOnError reporter as outlined above :+1:

@janraasch Isn't what he outlined above replicating gulp-coffeelint-threshold?

[janraasch]

@adam-lynch I was solely referring to the one comment (not our whole conversation) above where I outlined a possible implementation for a failOnError-reporter.

[adam-lynch]

@janraasch yeah but I'm saying if you don't want my plugin's functionality then why would you want his PR? Isn't it very very similar?

E.g.

.pipe(coffeelint.reporter('fail', {onWarnings: 0, onErrors: 5}))

versus

.pipe(coffeelintThreshold(10, 0, function(numberOfWarnings, numberOfErrors){
            gutil.beep();
            throw new Error('CoffeeLint failure; see above. Warning count: ' + numberOfWarnings
                + '. Error count: ' + numberOfErrors + '.');
        }));

[janraasch]

I do not want that pr at all, @adam-lynch. Could you please have a look at the specific comment https://github.com/janraasch/gulp-coffeelint/issues/11#issuecomment-38832526 I keep pointing to? I would take a pr containing something along the lines of

failOnErrorReporter = ->
    through2.obj (file, enc, cb) ->
        # any errors?
        if file.coffeelint?.errorCount
            @emit 'error', new Error "Found #{file.coffeelint.errorCount} CoffeeLint errors in #{file.relative}"
        @push file
        cb()

is all.

[janraasch]

Also, this issue is getting a little long (plus it has been closed for a long time), so I would ask @ejoubaud to take it from here:

Simply decide on whether you want to start a new issue or pullrequest on this repo or gulp-coffeelint-threshold depending on what fits best, and the discussion can then continue there in a focused manner.

[ejoubaud]

@adam-lynch Nothing I can't do with it, it's just that I like to avoid writing functions in my build scripts, keep it as close as possible to config as opposed to code, and I also don't like multiplying plugin dependencies.

@janraasch It's probably a stretch but would you be open to no-failure-on-warning being the default and I make the current one failOnWarning instead? TBH I was a bit surprised that warnings would fail the build by default, doesn't it defeat the purpose of having 2 levels of reporting if both end up triggering the same action? Things that issue warnings and errors (like compilers) usually only fail on the latter.

[janraasch]

@ejoubaud please open a new issue or pr with what you just said. Thank you very much :)

[adam-lynch]

@ejoubaud often when people compare gulp to grunt, they list pretty much the opposite of what you said as good things :smile:

gulp	grunt
code	config
small, idiomatic modules	plugins that do multiple things / too much
simple API	not as simple

@janraasch I guess this discussion is still on point. @ejoubaud is reiterating a lot of what I've already said.