Ignore unrecognized rules to maintain forward compatibility
Rantanen opened this issue · 18 comments
Currently jscs raises error if it encounters a rule it doesn't recognize. This causes problems when new rules, that old jscs versions do not recognize, are added to project's jscsrc file.
Obviously the recommended solution is for everyone to update their jscs at this point, but this isn't necessarily possible. Jscs might be bundled inside another product for example (e.g. Visual Studio Web Essentials).
I believe this is not a good approach: you think you are using declared rules, but some of them might be unsupported yet.
Yes, but these rules shouldn't prevent you from using JSCS completely.
JSHint contains a decent compromise: Report bad rules as errors just like any other error:
SelectionList.js: line 0, col 0, Bad option: 'inndent'.
SelectionList.js: line 15, col 10, 'x' is defined but never used.
@mdevils I like JSHint's approach as well.
I wanted to comment out some rules, like "// requireSpaceBeforeBlockStatements", but then it starts complaining. It's nice to be able to leave unsupported rules in. It would also be nice if you could explicitly set something to false.
+1 for JSHint's approach and +2 for comments and allowing to set an option to false.
(This would be a great help to comment your setup)
Comments should be already supported. At least with /* */ which I used yesterday unless I'm completely mistaken.
I'm guessing the false rules would be part of the planned 2.0 release which I've heard aims to combine certain rules:
requireSpacesInsideArrayBrackets: true-> spacesInsideArrayBrackets: true, disallowSpacesInsideArrayBrackets: true -> spacesInsideArrayBrackets: false,
unspecified: spacesInsideArrayBrackets: null
You can disable a rule with null, and you can comment out with /**/ in the latest versions. Please let me know if this works for you.
Yes it does work! Thank you.
@mdevils id like to implement the JSHint approach here. Once per file complain about unsupported rules on line 1, column 1 like we did for deprecated rules. This also allows us to remove the deprecated rules as they'll now just complain about being unsupported.
We can revisit in 2.0 if we want a better mechanism for reporting non code style errors, but right now the behavior of "just not working" if you're using an editor plugin is pretty bad.
Thoughts?
+1
@mikesherov I think it's a definitely a good approach, 👍 for it
@mikesherov I agree
Is this as simple as setting a new property like this._foundUnsupportedRules = true instead of throwing an error when hitting an unsupported rule.
Then, right before returning the errors, we check this._foundUnsupportedRules and add a new error (as you specified above) to the error list?
@mrjoelkemp it's even simpler. For each unsupported rule found, when it's found, add a single error, once for each rule.
But you don't have access to the error instance in StringChecker#configure, so you have to wait until StringChecker#checkString to add the errors for unsupported rules.
I don't see an alternative.
this._foundUnsupportedRules = true loses information. It should be one error per unsupported rule.
Why is this 2.0 and not 1.8? I'm moving it to 1.8, because it doesn't break backwards compatibility.