'merge-default-rules: false' ignored when set in config and not in options

[benthemonkey]

This is a really easy bug to fix.

This line: https://github.com/sasstools/sass-lint/blob/develop/lib/config.js#L77

should simply be an else.

I'll add a test for this and fix the issue.

[DanPurdy]

Nice one, If 1.3.0 is out by the time you get to this would your mind running this off the master branch? I think it warrants a hotfix.

Thanks!

[benthemonkey]

Lol, the tests for config aren't actually testing anything.

See here?
https://github.com/sasstools/sass-lint/blob/develop/tests/config.js#L37

equal isn't an assertion. You don't actually test if equal returns true.

[benthemonkey]

Update: It wasn't at all as simple as I thought it would be. I found many issues with the CLI tests, and by fixing those issues I think I'm causing some tests to fail. Not entirely sure.

Let me work on it a little bit more, and I'll ask for some help if I can't figure things out.

[DanPurdy]

Gah... my mind's gone.. I've fixed this small issue in a PR, for some reason it didn't register that I'd already read this somewhere as it was reported on my atom plugin and I went hunting!

I've left it as an else if just for specificity sake (open else's are evil?) but once you're happy with the tests on this run it straight into a PR for the develop branch or I can take over if you're getting bogged down on it? I'm just hoping to release this fix with 1.3.1 as it's probably leading to quite a few issues currently.

[benthemonkey]

No problem! I wasn't sure when I'd be able to get around to it. However, I don't think the problem is correctly fixed by your PR. If merge-default-rules is false, then none of the other options from the config file get merged, right?

See what I ended up doing: benthemonkey@e35e2b4

[DanPurdy]

Hmm, I see what you mean.. I think what you had makes sense, will re look at it later on.

[DanPurdy]

So i just had a look at this, i think my PR should fix the issue. If merge-default-rule:false is set in your custom config then the default rules should be completely ignored yes.

This custom config then essentially becomes the default rules. If you pass extra options through with merge default set to true then your extra options will either merge or overwrite your custom rules.

Are you seeing this as working differently?

[benthemonkey]

I agree that it fixes the issue about rules. What it doesn't do is fix the related issue of options not getting merged from config when merge-default-rules is false. If merge-default-rules is false but formatter is json (in config), sass-lint wont respect that, or any other option settings other than merge-default-rules: false.

[DanPurdy]

Ah yes, I see what you mean, if you set merge-default-rules to false it ignores your custom options however if you completely remove merge-default-rules it works fine..

[DanPurdy]

Ok this went out with 1.3.1. #318 I'm happy to tackle the test refactoring next.

[benthemonkey]

#310 Looks great! Closing.