Reimplement max-warnings as a proper option #857

nottrobin · 2016-08-31T17:51:45Z

What do the changes you have made achieve?

This adds max-warnings as a proper option, including tests and documentation. This option can be set in the config file, passed straight through to failOnError, or set via the command-line.

See #856 for more detail.

Are there any new warning messages?

No.

There's a new error message:

MaxWarningsExceededError: Number of warnings (257) exceeds the allowed maximum of 20.

Have you written tests?

Yes.

  failures
    ✓ should raise SassLintFailureError if indentation is set to error (595ms)
    ✓ should not raise SassLintFailureError if indentation is only set to warn
    ✓ should raise MaxWarningsExceededError if warnings exceed `max-warnings` setting (133ms)
    ✓ should not raise MaxWarningsExceededError if warnings do not exceed `max-warnings` setting

Have you included relevant documentation

Yes, in docs/options/max-warnings.md.

Which issues does this resolve?

Closes #856.

<DCO 1.1 Signed-off-by: Robin Winslow robin@robinwinslow.co.uk>

coveralls · 2016-09-01T01:07:42Z

Coverage increased (+0.3%) to 97.527% when pulling d7eae87 on nottrobin:max-warnings-option into 01eb696 on sasstools:develop.

coveralls · 2016-09-01T01:51:07Z

Coverage increased (+0.3%) to 97.527% when pulling 71a2917 on nottrobin:max-warnings-option into 01eb696 on sasstools:develop.

coveralls · 2016-09-01T02:07:34Z

Coverage increased (+0.3%) to 97.527% when pulling d57be3b on nottrobin:max-warnings-option into 01eb696 on sasstools:develop.

coveralls · 2016-09-01T02:07:34Z

Coverage increased (+0.3%) to 97.527% when pulling d57be3b on nottrobin:max-warnings-option into 01eb696 on sasstools:develop.

DanPurdy · 2016-09-01T08:12:01Z

docs/sass-lint.yml

@@ -9,6 +9,8 @@ options:
  formatter: html
  # Output file instead of logging results
  output-file: 'linters/sass-lint.html'
+  # Raise an error if more than 10 warnings are generated
+  max-warnings: 50


Set to 50 but the comment mentions 10

Good catch. Fixed.

coveralls · 2016-09-01T08:16:40Z

Coverage increased (+0.3%) to 97.527% when pulling ac6a8fe on nottrobin:max-warnings-option into 01eb696 on sasstools:develop.

DanPurdy · 2016-09-01T08:34:42Z

Looking good @nottrobin, can we add a few test cases for when the user specifically sets max-warnings to 0 in their config file. I can see that it will pass but just belts and braces for the future.

As a side note we will need to remember to replace that util.inherits when we drop support for earlier versions of node as they come off of LTS.

Thanks! 👍

nottrobin · 2016-09-01T08:58:17Z

Good idea. I'll get those new tests written in the next couple of hours.

On Thu, 1 Sep 2016, 09:34 Dan Purdy, notifications@github.com wrote:

Looking good @nottrobin https://github.com/nottrobin, can we add a few
test cases for when the user specifically sets max-warnings to 0 in their
config file. I can see that it will pass but just belts and braces for the
future.

As a side note we will need to remember to replace that util.inherits
when we drop support for earlier versions of node as they come off of LTS.

Thanks! 👍

—
You are receiving this because you were mentioned.

Reply to this email directly, view it on GitHub
#857 (comment),
or mute the thread
https://github.com/notifications/unsubscribe-auth/AAfu_76dI-5qhOuJiFG_05Ol28Qj4S8hks5qlo4kgaJpZM4Jx4i-
.

DanPurdy · 2016-09-01T09:01:39Z

No rush, got a few other things to finish and add in 1.10 anyway.

coveralls · 2016-09-01T14:01:32Z

Coverage increased (+0.3%) to 97.527% when pulling 4fd8964 on nottrobin:max-warnings-option into 01eb696 on sasstools:develop.

nottrobin · 2016-09-01T14:20:05Z

@DanPurdy I've now added two new tests:

should raise MaxWarningsExceededError if warnings exceed max-warnings of zero
should not raise error if no warnings even if max-warnings is zero

DanPurdy · 2016-09-01T16:16:07Z

tests/failures.js

+
+  it('should not raise error if no warnings even if `max-warnings` is zero', function (done) {
+    var results = lint.lintFiles('sass/success.scss', {});  // no warnings
+    lint.failOnError(results, {'max-warnings': 0}); // Max 100 warnings, should succceed


The comments are still for max 100 tests here

Good point, again. I'll fix this.

DanPurdy · 2016-09-01T16:16:56Z

Thanks just the copy n pasted comments to deal with and then this lgtm for me!

👍

- Remove custom, slightly hacky code for `max-warnings` from `bin/sass-lint.js` - Tidy up `bin/sass-lint.js` more generally - Add `max-warnings` as an option to be passed to `failOnError`, either through a config file or through passing options directly - Add documentation for the max-warnings option - Create new descriptive exceptions for `failOnError` - MaxWarningsExceededError and SassLintFailureError - Test the functionality of `failOnError`

coveralls · 2016-09-01T22:55:53Z

Coverage increased (+0.3%) to 97.527% when pulling fbaf738 on nottrobin:max-warnings-option into 01eb696 on sasstools:develop.

DanPurdy · 2016-09-02T10:49:36Z

Awesome, thanks @nottrobin looking good. Will come out with 1.10 in the next week or two hopefully

DanPurdy · 2016-11-07T15:12:25Z

Sorry @nottrobin I seemed to miss a unfortunate outcome from this to do with silencing errors and preventing unhandled exceptions with the --no-exit flag on the cli which lead to this change always throwing an unhandled error if a rule of severity 2 detected an issue.

I've had to sort of half patch the old functionality on top of yours so you should still encounter exit code 1 when max-warnings fails but the error itself won't be thrown if you're specifiying --no-exit or -q.

Let me know if there's any issues here and we can work a solution out.

nottrobin mentioned this pull request Aug 31, 2016

Make "max-warnings" a proper option #856

Closed

nottrobin force-pushed the max-warnings-option branch from 30f6e6e to e09df5e Compare August 31, 2016 18:54

nottrobin force-pushed the max-warnings-option branch 2 times, most recently from 24d793c to 71a2917 Compare September 1, 2016 01:48

nottrobin force-pushed the max-warnings-option branch from 71a2917 to d57be3b Compare September 1, 2016 02:04

nottrobin mentioned this pull request Sep 1, 2016

Introducing sass lint canonical/vanilla-framework#534

Merged

DanPurdy reviewed Sep 1, 2016
View reviewed changes

nottrobin force-pushed the max-warnings-option branch from d57be3b to ac6a8fe Compare September 1, 2016 08:13

DanPurdy added the ready for review label Sep 1, 2016

DanPurdy added this to the 1.10 milestone Sep 1, 2016

nottrobin force-pushed the max-warnings-option branch from ac6a8fe to 4fd8964 Compare September 1, 2016 13:58

DanPurdy reviewed Sep 1, 2016
View reviewed changes

nottrobin force-pushed the max-warnings-option branch from 4fd8964 to fbaf738 Compare September 1, 2016 22:52

DanPurdy merged commit b7bcb3a into sasstools:develop Sep 2, 2016

DanPurdy mentioned this pull request Nov 7, 2016

Fix cli error throws #937

Merged

nottrobin deleted the max-warnings-option branch November 7, 2016 15:10

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Reimplement max-warnings as a proper option #857

Reimplement max-warnings as a proper option #857

nottrobin commented Aug 31, 2016 •

edited

Loading

coveralls commented Sep 1, 2016 •

edited

Loading

coveralls commented Sep 1, 2016 •

edited

Loading

coveralls commented Sep 1, 2016 •

edited

Loading

coveralls commented Sep 1, 2016

DanPurdy Sep 1, 2016

nottrobin Sep 1, 2016

coveralls commented Sep 1, 2016 •

edited

Loading

DanPurdy commented Sep 1, 2016

nottrobin commented Sep 1, 2016

DanPurdy commented Sep 1, 2016

coveralls commented Sep 1, 2016 •

edited

Loading

nottrobin commented Sep 1, 2016

DanPurdy Sep 1, 2016

nottrobin Sep 1, 2016

nottrobin Sep 1, 2016

DanPurdy commented Sep 1, 2016

coveralls commented Sep 1, 2016 •

edited

Loading

DanPurdy commented Sep 2, 2016 •

edited

Loading

DanPurdy commented Nov 7, 2016

Reimplement max-warnings as a proper option #857

Reimplement max-warnings as a proper option #857

Conversation

nottrobin commented Aug 31, 2016 • edited Loading

coveralls commented Sep 1, 2016 • edited Loading

coveralls commented Sep 1, 2016 • edited Loading

coveralls commented Sep 1, 2016 • edited Loading

coveralls commented Sep 1, 2016

DanPurdy Sep 1, 2016

Choose a reason for hiding this comment

nottrobin Sep 1, 2016

Choose a reason for hiding this comment

coveralls commented Sep 1, 2016 • edited Loading

DanPurdy commented Sep 1, 2016

nottrobin commented Sep 1, 2016

DanPurdy commented Sep 1, 2016

coveralls commented Sep 1, 2016 • edited Loading

nottrobin commented Sep 1, 2016

DanPurdy Sep 1, 2016

Choose a reason for hiding this comment

nottrobin Sep 1, 2016

Choose a reason for hiding this comment

nottrobin Sep 1, 2016

Choose a reason for hiding this comment

DanPurdy commented Sep 1, 2016

coveralls commented Sep 1, 2016 • edited Loading

DanPurdy commented Sep 2, 2016 • edited Loading

DanPurdy commented Nov 7, 2016

nottrobin commented Aug 31, 2016 •

edited

Loading

coveralls commented Sep 1, 2016 •

edited

Loading

coveralls commented Sep 1, 2016 •

edited

Loading

coveralls commented Sep 1, 2016 •

edited

Loading

coveralls commented Sep 1, 2016 •

edited

Loading

coveralls commented Sep 1, 2016 •

edited

Loading

coveralls commented Sep 1, 2016 •

edited

Loading

DanPurdy commented Sep 2, 2016 •

edited

Loading