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

Make "max-warnings" a proper option #856

Closed
nottrobin opened this issue Aug 31, 2016 · 4 comments · Fixed by #857
Closed

Make "max-warnings" a proper option #856

nottrobin opened this issue Aug 31, 2016 · 4 comments · Fixed by #857

Comments

@nottrobin
Copy link
Contributor

nottrobin commented Aug 31, 2016

The command-line tool provides the --max-warnings option. This is very useful for preventing a codebase from deteriorating and generating ever more warnings.

However, we can't use this feature for vanilla-framework as we run sass-lint through gulp-sass-lint in our gulpfile.js instead of using the CLI.

As far as I can tell there's no way to set program.maxWarnings through any other method. It would be most helpful to us if this could be an option in the same sense as formatter et al., so we can set it within the .sass-lint.yml config file as follows:

options:
  max-warnings: 25
@nottrobin
Copy link
Contributor Author

I'll have a little crack at writing this myself. Although if I can't get the bulk of the way there tonight I'll probably abandon it. 😝

@DanPurdy
Copy link
Member

Hi @nottrobin,
The difference here is that the CLI just throws an exit code of 1 if it goes over a certain number of warnings, this wouldn't be the case with gulp as you need to still report on every warning/file and then hand it off downstream. Would you expect it to throw an extra error message if it went over the number of warnings you specify?
I'm not too clear on the use case here and what the intended outcome would be.

@nottrobin
Copy link
Contributor Author

nottrobin commented Aug 31, 2016

Thanks for the quick response @DanPurdy.

So currently, if I were to set indentation: 2 in my .sass-lint.yml file, break indentation in one of my sass files thus:

ul {
    margin-left: 1rem;
  margin-bottom: 1rem;
}

and run sass-lint, I see an exception with an error message, and, as you say, an exit code of 1:

$ sass-lint
...
Error: 1 errors were detected in
- scss/_base_lists.scss
...

Similarly, if I have a gulpfile.js which includes the following:

var sassLint = require('gulp-sass-lint');

gulp.task('lint:sass', function() {
  return gulp.src('scss/**/*.s+(a|c)ss')
    .pipe(sassLint())
    .pipe(sassLint.format())
    .pipe(sassLint.failOnError())
});

And I run this target with gulp, I see an equivalent error, with an exit code of 1.

$ gulp lint:sass
...
[18:10:23] 'lint:sass' errored after 1.11 s
[18:10:23] Error in plugin 'sass-lint'
Message:
    1 errors detected in _base_lists.scss

We use gulp in our CI testing, and therefore rely on the exit code of 1 that it produces on an error.

I would expect very similar behaviour from a max-warnings option - that is that it generates an error message and an exit code of 1 if max-warnings is exceeded, whether it's run through the CLI or through gulp.

Currently if I run sass-lint --max-warnings 10 I get an exit code of 1, but no error message which is a shame. But crucially I'd like to be able to generate an exit code of 1 for the same reason through the .sass-lint.yml file.

I've had a go at developing this, in #857, so you can see what I mean. It's now ready and all checks have passed.

@nottrobin
Copy link
Contributor Author

nottrobin commented Sep 1, 2016

I have just discovered that this won't solve my problem with gulp-sass-lint, as instead of using sass-lint's failOnError function, it reimplements its own version.

Once this is solved in sass-lint, I'd be happy to submit a follow-up PR to gulp-sass-lint to make use of the native failOnError method and so properly support the max-warnings option.

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

Successfully merging a pull request may close this issue.

2 participants