Errors.add should throw on invalid calls

[qfox]

To simplify rules debugging, prevent dumb mistakes and crashes in error reporters.

[coveralls]

Coverage decreased (-0.03%) when pulling ee4dfd6 on zxqfox:hotfix/prevent-crashes-in-error-reporters into cda4d10 on jscs-dev:master.

[mrjoelkemp]

Why the '(column & & ' clause?

[qfox]

Oh, because it's declared as optional in jsdoc. Probably it isn't right.

[mrjoelkemp]

K. Makes sense. Please comment above that line with that explanation.

Can you add a test for this please?

[qfox]

Sure, but I don't know where to place it. In string-checkers?
Also test will look like:

        function makeThrowingEsprima(description, line, column) {
            return {
                parse: function() {
                    var error = new Error(description);
                    error.lineNumber = line;
                    error.column = column;
                    throw error;
                }
            };
        };

        it('should throw an error on errors.add', function() {
            checker = new Checker({ esprima: makeThrowingEsprima('object') });
            checker.registerDefaultRules();

            var errors = checker.checkString('import { foo } from "bar";');
            var error = errors.getErrorList()[0];

            // check error...
        });

Right?

[mdevils]

Oh, because it's declared as optional in jsdoc. Probably it isn't right.

If it is optional, write column === undefined, don't go with implicit boolean conversion.

[mdevils]

Invalid error location data: <' + message + '> at <' + line + '>, <' + column + '>

I think this is a bad error message.

I would go for more specific errors like "Unable to add an error, column should be a number", "Unable to add an error, line should be a number".

[qfox]

Sure I'll change message.

It was a useful proposal with minimal functionality.

[qfox]

@mdevils @mrjoelkemp Guys, I'm done. Need review.

[coveralls]

Coverage increased (+0.01%) when pulling daded70 on zxqfox:hotfix/prevent-crashes-in-error-reporters into cda4d10 on jscs-dev:master.

[mrjoelkemp]

I'm not sure why makeThrowingEsprima is necessary. Why can't you just call errors.add with arguments that trigger the exception?

Something like:

assert.throws(function() {
  errors.add('foo', -1, -1);
})

All of the arguments that you passed to makeThrowingEsprima could be passed to errors.add instead.

Would that not work?

[qfox]

Well, yes. Idk why I did it like that. Gonna fix that.

[qfox]

@mrjoelkemp fixed, thanks!

[coveralls]

Coverage increased (+0.01%) when pulling 1fe8870 on zxqfox:hotfix/prevent-crashes-in-error-reporters into f68fb06 on jscs-dev:master.

[mdevils]

Great job. The last thing, can you use assert module there please.

Example: https://github.com/jscs-dev/node-jscs/blob/master/lib/config/configuration.js#L200

[qfox]

@mdevils Sure. I was afraid it's not possible to use there. Going to do that.

[coveralls]

Coverage increased (+0.01%) when pulling 4f1a14b on zxqfox:hotfix/prevent-crashes-in-error-reporters into f68fb06 on jscs-dev:master.

[mdevils]

Looks good to me.

@mrjoelkemp, @markelog, OK to merge?

[qfox]

@mdevils I would squash before merging. Should I?

[mdevils]

Yep :)

[qfox]

Done.

[coveralls]

Coverage increased (+0.01%) when pulling 1ac6e34 on zxqfox:hotfix/prevent-crashes-in-error-reporters into f68fb06 on jscs-dev:master.

[mrjoelkemp]

@mdevils looks good. feel free to merge.

@zxqfox Great job! Thanks!

[mdevils]

Merged, thanks!

[qfox]

You are welcome 🌴