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

implement failOnWarning #72

Closed

Conversation

chris-rock
Copy link

The addition of failOnWarning allows gulp to return an exit code for warnings:

$ gulp sasslint
[13:35:22] Using gulpfile ~/Development/test/gulpfile.js
[13:35:22] Starting 'sasslint'...
[13:35:22] Finished 'sasslint' after 8.62 ms

test/report/report.scss
   8:3   warning  Expected `display`, found `font-family`  property-sort-order
  20:12  warning  Trailing semicolons required             trailing-semicolon

✖ 2 problems (0 errors, 2 warnings)


events.js:160
      throw er; // Unhandled 'error' event
      ^
Error: 0 warnings detected in test/report.scss
$ echo $?
1

This becomes very handy if gulp is used in CI environments where we want to prevent a merge if the style guide is not met

@Snugug
Copy link
Member

Snugug commented Jan 24, 2017

Hey @chris-rock, thanks for the PR!

When talking about the semantics of warnings vs errors, warnings are parts of a style guide that you'd prefer not to se written, but aren't explicitly disallowed by the style guide. Errors are things that are explicitly disallowed by the style guide. If you're looking to error your CI when a rule is not met, those should be errors, not warnings, in your style guide.

Because of this, I'm not going to merge this PR as it breaks the semantic difference between a warning and an error, and we want to be consistent here across Sass Lint and other linters that use similar semantics. My recommendation is to change those warnings you have that you don't want to be allowed to errors.

@Snugug Snugug closed this Jan 24, 2017
@chris-rock
Copy link
Author

chris-rock commented Jan 24, 2017

@Snugug I am not sure why you are not allowing a discussion. But then the semantics in sass-lint are not right:

For all rules, setting their severity to 0 turns it off, setting to 1 sets it as a warning
(something that should not be committed in), and setting to 2 sets it to an error
(something that should not be written)

This is from https://github.com/sasstools/sass-lint#rules. How should I prevent 1, if the gulp tooling does not allow me to catch this?

@chris-rock
Copy link
Author

#73

@chris-rock
Copy link
Author

I move the discussion to #73. I'd like to highlight, that I would appreciate a proper discussion instead of harsh behavior.

@opr
Copy link

opr commented Feb 24, 2017

Agree with @chris-rock here - regardless of your religious beliefs regarding warnings and errors, the option to fail on warnings would be lovely, if you want to stop code that generates warnings being merged into your production code then you can enable it.

@lieldulev
Copy link

Regardless of semantics (on which I agree with @Snugug) - max-warnings should be respected, since you might have a threshold of warnings you are okay with and since sass lint supports it, one would assume gulp sass lint should respect that.

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.

4 participants