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

Fix cli error throws #937

Merged
merged 3 commits into from
Nov 7, 2016

Conversation

DanPurdy
Copy link
Member

@DanPurdy DanPurdy commented Nov 7, 2016

With the introduction of the max-warnings option for config files in #857 an error was introduced where any errors detected by sass-lint would always throw an unhandled exception even if the --no-exit / -q option were specified.

This was unintended.

The following should now be true for all CLI instances

  • If -q then all unhandled errors and exits should be quiet, an error code of 1 will still be present
  • If max warnings fails and the CLI is 'quiet' then no errors will be thrown but an error code of 1 will be present
  • If not quiet then any errors detected (i.e. rules set to 2) will throw an exception and exit the process on the first error detected.
  • If not quiet and no errors are detected but the max-warnings option is exceeded then a max-warnings exception will be thrown

@nottrobin you may want to give this a once over too as it may affect your initial intentions with the PR you made. Unfortunately without a major version bump I don't think the exact implementation of yours would work, sorry that I missed this eventuality at the time.

<DCO 1.1 Signed-off-by: Dan Purdy [email protected]>

@coveralls
Copy link

coveralls commented Nov 7, 2016

Coverage Status

Coverage remained the same at 97.548% when pulling f174738 on DanPurdy:feature/fix-cli-error-throws into 0fbf9bd on sasstools:develop.

1 similar comment
@coveralls
Copy link

Coverage Status

Coverage remained the same at 97.548% when pulling f174738 on DanPurdy:feature/fix-cli-error-throws into 0fbf9bd on sasstools:develop.

@nottrobin
Copy link
Contributor

nottrobin commented Nov 7, 2016

@DanPurdy Thanks for flagging me here. Sorry for introducing this bug. It sounds like you've barely changed the actual interface from what I created - the only difference is that you're suppressing error output if -q is specified, as you should. Is that correct? That's fine with me.

@DanPurdy
Copy link
Member Author

DanPurdy commented Nov 7, 2016

No problem, It was actually a weak test of mine that wasn't flagging this. Exactly, haven't changed too much just had to go a bit of a roundabout way to achieve it. Just wanted to make sure you were aware in case it causes you issues 👍

@coveralls
Copy link

Coverage Status

Coverage remained the same at 97.554% when pulling 115cb87 on DanPurdy:feature/fix-cli-error-throws into 60624c2 on sasstools:develop.

1 similar comment
@coveralls
Copy link

coveralls commented Nov 7, 2016

Coverage Status

Coverage remained the same at 97.554% when pulling 115cb87 on DanPurdy:feature/fix-cli-error-throws into 60624c2 on sasstools:develop.

@DanPurdy DanPurdy merged commit 240a148 into sasstools:develop Nov 7, 2016
@DanPurdy DanPurdy deleted the feature/fix-cli-error-throws branch November 7, 2016 15:32
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants