Adding addRule function

[lukasoppermann]

Users should be able to add their own rules, just like with the csslint plugin. This solves #24

[SimenB]

@lukasoppermann Code style check failed. Seems I messed up in #22 and don't fail the build...

[lukasoppermann]

@SimenB sorry, I don't get it, do I need to fix/change something?

[SimenB]

run npm test locally and you'll see an error. The build should have failed, though 😄

[lukasoppermann]

@SimenB ahh, now I get it, fixed the missing space. thanks.

[SimenB]

@lukasoppermann Seems there's no rule for it, but braces in general are on same line as conditionals in this source code.

No JSCS rule for it though...

if (condition) {

not

if (condition)
{

@lazd I just hijacked this, whoop! Could you update the rc's to be more strict maybe? Or do you want me to do it?

[lukasoppermann]

@SimenB so how is else?

Option 1:

} else {

or Option 2:

}
else {

[SimenB]

@lukasoppermann That I don't know. Doesn't seem to be any other elses...
@lazd will have to answer that one.

How about adding "disallowNewlineBeforeBlockStatements": true to .jscsrc?

[lukasoppermann]

@SimenB I just removed the need for else.

[SimenB]

@lukasoppermann Ah, nice!

[lazd]

@lukasoppermann Add some documentation to README and we can merge this!

[coveralls]

Coverage decreased (-2.61%) to 63.49% when pulling 921887f on lukasoppermann:patch-1 into 2a616d2 on lazd:master.

[coveralls]

Coverage decreased (-2.61%) to 63.49% when pulling 10e14f5 on lukasoppermann:patch-1 into 2a616d2 on lazd:master.

[lukasoppermann]

Hey @lazd I added the docs, but the coverage stuff now freaked. :) Wasn't there before, I just changed the readme.

[SimenB]

Yeah, we just added test-coverage from Coveralls yesterday. You could add a test? 😃

[lukasoppermann]

@SimenB had a look but I have no idea how to write a test for this. I do not have much experience with gulp plugins.

[lazd]

I think the best way to test this is to provide a fixture that passes with the default ruleset, but fails with a rule that you add using the new API. Then, run that fixture against the plugin and expect an error.

[lukasoppermann]

Hey, my problem is rather that I can not write the complete test, because writing a test for a gulp plugin is something I don't know how to do. Can you just merge it and write the test? You seem to have a pretty good idea of how to do it, so I guess you could wip up a test within a minute or so.

[lazd]

@lukasoppermann, it's pretty easy, and there's no better time to learn than now :) Just look at the existing tests and modify one to fit your needs.

This test expects something to fail, so you can copy paste it, use a different fixture, invoke your new addRule function inside of it, and be done.

[lukasoppermann]

@lazd I added a test, but I can't get it to work the whole gulp thing is just over the top for me.
It should fail if your class name does not start with something like .o-object but it always passes. ;(

Please help.

[coveralls]

Coverage increased (+0.56%) to 66.67% when pulling bd258b2 on lukasoppermann:patch-1 into 2a616d2 on lazd:master.

[coveralls]

Coverage increased (+0.56%) to 66.67% when pulling bd258b2 on lukasoppermann:patch-1 into 2a616d2 on lazd:master.

[SimenB]

@lukasoppermann Does it fail runtime? As in actually adding that rule and running against a real codebase?

I'm really bad at regexp, but from what I understand, the last part (allowing alphanumerics) is wrong, and only matches one char.

'.o-foo'.match(/^\.(_)?(o|c|u|is|has|js|qa)-[a-z0-9]$/)
 > null

'.o-f'.match(/^\.(_)?(o|c|u|is|has|js|qa)-[a-z0-9]$/)
 > [".o-f", undefined, "o"]

EDIT: Adding a plus at the end makes it pass.

'.o-foo'.match(/^\.(_)?(o|c|u|is|has|js|qa)-[a-z0-9]+$/)
 > [".o-foo", undefined, "o"]

But fail at other

'.bogus-foo'.match(/^\.(_)?(o|c|u|is|has|js|qa)-[a-z0-9]+$/)
 > null

Note: Not tested in your code/CSSLint, just using Node.js console

[lukasoppermann]

@SimenB thanks for catching it. Actually my problem was a wrong css file. This is why it did not report and error, which means not passing the test. :)

[SimenB]

Ah, OK. Nice you got it running.

Travis seems to have problems npm installing atm

[coveralls]

Coverage increased (+0.56%) to 66.67% when pulling 1b07d90 on lukasoppermann:patch-1 into 2a616d2 on lazd:master.

[lazd]

I restarted the build and all is well. Looks good, thanks for the contribution!

[lukasoppermann]

Hey, could you maybe release a version with this merge in it?