CLI: Support a maxErrors option to limit the number of reported errors (fixes #664, fixes #238) #673

mdevils · 2014-10-08T18:09:24Z

I started on #664 and remade this to be simpler and non-static.

coveralls · 2014-10-08T18:14:19Z

Coverage increased (+0.02%) when pulling bbff037 on mdevils.maxerr into bd62246 on master.

coveralls · 2014-10-08T18:21:25Z

Coverage increased (+0.02%) when pulling 44d0deb on mdevils.maxerr into bd62246 on master.

mikesherov · 2014-10-08T18:47:41Z

This seems far more confusing to me than the other pull request. The math and reaching into prototypes of parent simply to avoid a shared value between error instances seems like unnecessary contortion.

Furthermore, this unnecessarily checks each file even after the limit is already hit.

mikesherov · 2014-10-08T18:49:19Z

lib/checker.js

+ * @returns {Errors}
+ */
+Checker.prototype.checkString = function(str, filename) {
+    var errors = StringChecker.prototype.checkString.apply(this, arguments);


is there any reason not to move this behavior up into StringChecker?

mikesherov · 2014-10-08T18:58:17Z

Still, I think this implementation IS cleaner than the @mrjoelkemp version.

mdevils · 2014-10-08T20:07:25Z

The math and reaching into prototypes of parent simply to avoid a shared value between error instances seems like unnecessary contortion.

Moved the logic into StringChecker. Its looks simpler, thank you!

Furthermore, this unnecessarily checks each file even after the limit is already hit.

Fixed this.

coveralls · 2014-10-08T20:08:50Z

Coverage increased (+0.06%) when pulling 8eee218 on mdevils.maxerr into bd62246 on master.

markelog · 2014-10-08T22:00:29Z

lib/cli.js

@@ -135,6 +139,10 @@ module.exports = function(program) {

            reporter(errorsCollection);

+            if (checker.maxErrorsExceeded()) {
+                console.log('Too many errors... Increase `maxErrors` configuration option value to see more.');


So this would not come up if user doesn't use TTY?

+1. Nice work, Marat.
On Oct 8, 2014 6:13 PM, "Oleg Gaidarenko" notifications@github.com wrote:

In lib/cli.js:

@@ -135,6 +139,10 @@ module.exports = function(program) {

reporter(errorsCollection);

if (checker.maxErrorsExceeded()) {

console.log('Too many errors... Increase `maxErrors` configuration option value to see more.');

So this would not come up if user doesn't use TTY?

—
Reply to this email directly or view it on GitHub
https://github.com/jscs-dev/node-jscs/pull/673/files#r18615530.

So this would not come up if user doesn't use TTY?

You mean browser version? If so, we don't even include reporters there. Users are free to implement their own reactions.

No, when dealing with piped stdin input. We should show this error message
too
On Oct 9, 2014 6:02 AM, "Marat Dulin" notifications@github.com wrote:

In lib/cli.js:

@@ -135,6 +139,10 @@ module.exports = function(program) {

reporter(errorsCollection);

if (checker.maxErrorsExceeded()) {

console.log('Too many errors... Increase `maxErrors` configuration option value to see more.');

So this would not come up if user doesn't use TTY?

You mean browser version? If so, we don't even include reporters there.
Users are free to implement their own reactions.

—
Reply to this email directly or view it on GitHub
https://github.com/jscs-dev/node-jscs/pull/673/files#r18636360.

I still think the max errors exceeded itself should be an error, like in JSHint.

In fact, I still contend that all info from JSCS should be reported as style violations, like we did when deprecated rules or converted the errors from esprima.

markelog · 2014-10-08T22:05:04Z

/cc @mrjoelkemp

mrjoelkemp · 2014-10-10T02:07:20Z

When can we land this? Should we land a follow up commit fixing the stdin issue and adding tests? 1.7 is long overdue.

mikesherov · 2014-10-10T02:16:54Z

Seems like we just need to resolve how to display the maxError message. Release tomorrow?

mdevils · 2014-10-10T10:14:49Z

Added message display call to STDIN-handling code.

Is it correct?

If so, I'm ready to merge.

coveralls · 2014-10-10T10:17:09Z

Coverage increased (+0.02%) when pulling d00b31c on mdevils.maxerr into d0150e0 on master.

mrjoelkemp · 2014-10-10T11:04:58Z

Looks good. I'll file an issue to write a test for the stdin case, but it looks fine as is. Nice work! So excited for the release :)

mdevils · 2014-10-10T11:28:52Z

OK) Merging.

fixes #664, fixes #238)

markelog · 2014-10-10T15:26:46Z

There was comment by @mikesherov somewhere with similar idea - instead writing maxerror error in the console we could add it as an error with values (line, column) of the previous error.

That way every reporters would work as of now but user would know (even in CI run) that JSCS didn't check everything and bail out.

Which allow us to change default value of maxerror property.

How is that sounds?

mdevils · 2014-10-13T19:42:50Z

@markelog how would it stop us from breaking backwards compatibility?

mikesherov · 2014-10-13T20:59:21Z

Any error we could report as a style violation is better than any error we report only in some reporters or as exceptions.

By reporting the 51st error as a line violation would allow us to stop early without having the user possibly not see the reason why.

In this way, we can be confident in lower the default max error without leaving our users in the dark. We could change the default, and be confident users would know how to fix it should they want to disable maxErrors.

markelog · 2014-10-14T08:51:44Z

@mikesherov yep, exactly my train of thought

mrjoelkemp · 2014-10-14T12:22:39Z

Agreed about the benefits of showing maxerr as an error. Is this a blocker for 1.7? Could this follow up change be pushed to 1.8?

markelog · 2014-10-14T12:29:36Z

Agreed about the benefits of showing maxerr as an error.

This change should not be that hard to implement

mdevils · 2014-10-14T12:44:26Z

I agree with @mikesherov about reporting style about this error.

But right now I'm talking about current CI-integrations and Instruments (like Sublime integration) which expect all errors to be reported. For them changing this option value from Infinity to something else will be a compatibility breaking change.

mikesherov · 2014-10-14T12:55:24Z

According to our release semantics that we agreed upon:

Minor Release:

Modifying rules so they report less errors, and don't cause build
failures.

Therefore, it is perfectly acceptable to reduce maxErrors to whatever
number we feel is appropriate in 1.7. We do not need to wait for 2.0 for
this

On Tue, Oct 14, 2014 at 8:44 AM, Marat Dulin notifications@github.com
wrote:

I agree with @mikesherov https://github.com/mikesherov about reporting
style about this error.

But right now I'm talking about current CI-integrations and Instruments
(like Sublime integration) which expect all errors to be reported. For them
changing this option value from Infinity to something else will be a
compatibility breaking change.

Reply to this email directly or view it on GitHub
#673 (comment).

Mike Sherov

mrjoelkemp · 2014-10-14T13:12:25Z

Should we say 1.7 ships with no default (it's opt-in) and 2.0 lowers the
default to 50?
On Oct 14, 2014 8:44 AM, "Marat Dulin" notifications@github.com wrote:

I agree with @mikesherov https://github.com/mikesherov about reporting
style about this error.

But right now I'm talking about current CI-integrations and Instruments
(like Sublime integration) which expect all errors to be reported. For them
changing this option value from Infinity to something else will be a
compatibility breaking change.

—
Reply to this email directly or view it on GitHub
#673 (comment).

markelog · 2014-10-14T13:15:45Z

I guess, this not applicable for 1.7, since we changed and fixed a lot before accepting this protocol, we probably should use this logic for the next releases but not for 1.7.

But right now I'm talking about current CI-integrations and Instruments (like Sublime integration) which expect all errors to be reported.

hmm, that wouldn't be a problem if this option wouldn't be used for the whole run, but for individual files.
Anyway, something to think about.

CI-integrations

They will definitely see this last problem and could react accordingly, in my head they do :-)

Sublime integration

Those plugins lint one file at the time, so they gonna see plenty, might be a problem though.

markelog · 2014-10-14T13:16:33Z

and btw @mikesherov
it says "Modifying rules so they report less errors, and don't cause build
failures."

Modifying rules not options.

markelog · 2014-10-14T13:19:12Z

Let's agree on change the default for 2.0 and discuss whole run vs individual files?

mikesherov · 2014-10-14T13:28:07Z

Modifying rules not options.

This is a difference without a distinction. The effect is the same.

Let's agree on change the default for 2.0 and discuss whole run vs individual files?

Fair enough.

mikesherov · 2014-10-14T13:28:47Z

#681

markelog · 2014-10-14T17:35:36Z

#681

cool

mdevils mentioned this pull request Oct 8, 2014

CLI: Support a maxErrors option to limit the number of reported errors #664

Closed

3 tasks

mdevils force-pushed the mdevils.maxerr branch from bbff037 to 44d0deb Compare October 8, 2014 18:14

mikesherov reviewed Oct 8, 2014
View reviewed changes

mdevils force-pushed the mdevils.maxerr branch 2 times, most recently from 24386f3 to 8eee218 Compare October 8, 2014 20:06

markelog reviewed Oct 8, 2014
View reviewed changes

markelog added cli new option labels Oct 10, 2014

mdevils force-pushed the mdevils.maxerr branch from 8eee218 to d00b31c Compare October 10, 2014 10:13

CLI: Support a maxErrors option to limit the number of reported errors (

bbf85df

fixes #664, fixes #238)

mdevils force-pushed the mdevils.maxerr branch from d00b31c to bbf85df Compare October 10, 2014 11:29

mdevils merged commit bbf85df into master Oct 10, 2014

mdevils deleted the mdevils.maxerr branch October 10, 2014 11:32

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

CLI: Support a maxErrors option to limit the number of reported errors (fixes #664, fixes #238) #673

CLI: Support a maxErrors option to limit the number of reported errors (fixes #664, fixes #238) #673

mdevils commented Oct 8, 2014

coveralls commented Oct 8, 2014

coveralls commented Oct 8, 2014

mikesherov commented Oct 8, 2014

mikesherov Oct 8, 2014

mikesherov commented Oct 8, 2014

mdevils commented Oct 8, 2014

coveralls commented Oct 8, 2014

markelog Oct 8, 2014

mrjoelkemp Oct 8, 2014

mdevils Oct 9, 2014

mrjoelkemp Oct 9, 2014

mikesherov Oct 9, 2014

markelog commented Oct 8, 2014

mrjoelkemp commented Oct 10, 2014

mikesherov commented Oct 10, 2014

mdevils commented Oct 10, 2014

coveralls commented Oct 10, 2014

mrjoelkemp commented Oct 10, 2014

mdevils commented Oct 10, 2014

markelog commented Oct 10, 2014

mdevils commented Oct 13, 2014

mikesherov commented Oct 13, 2014

markelog commented Oct 14, 2014

mrjoelkemp commented Oct 14, 2014

markelog commented Oct 14, 2014

mdevils commented Oct 14, 2014

mikesherov commented Oct 14, 2014

mrjoelkemp commented Oct 14, 2014

markelog commented Oct 14, 2014

markelog commented Oct 14, 2014

markelog commented Oct 14, 2014

mikesherov commented Oct 14, 2014

mikesherov commented Oct 14, 2014

markelog commented Oct 14, 2014

CLI: Support a maxErrors option to limit the number of reported errors (fixes #664, fixes #238) #673

CLI: Support a maxErrors option to limit the number of reported errors (fixes #664, fixes #238) #673

Conversation

mdevils commented Oct 8, 2014

coveralls commented Oct 8, 2014

coveralls commented Oct 8, 2014

mikesherov commented Oct 8, 2014

mikesherov Oct 8, 2014

Choose a reason for hiding this comment

mikesherov commented Oct 8, 2014

mdevils commented Oct 8, 2014

coveralls commented Oct 8, 2014

markelog Oct 8, 2014

Choose a reason for hiding this comment

mrjoelkemp Oct 8, 2014

Choose a reason for hiding this comment

mdevils Oct 9, 2014

Choose a reason for hiding this comment

mrjoelkemp Oct 9, 2014

Choose a reason for hiding this comment

mikesherov Oct 9, 2014

Choose a reason for hiding this comment

markelog commented Oct 8, 2014

mrjoelkemp commented Oct 10, 2014

mikesherov commented Oct 10, 2014

mdevils commented Oct 10, 2014

coveralls commented Oct 10, 2014

mrjoelkemp commented Oct 10, 2014

mdevils commented Oct 10, 2014

markelog commented Oct 10, 2014

mdevils commented Oct 13, 2014

mikesherov commented Oct 13, 2014

markelog commented Oct 14, 2014

mrjoelkemp commented Oct 14, 2014

markelog commented Oct 14, 2014

mdevils commented Oct 14, 2014

mikesherov commented Oct 14, 2014

mrjoelkemp commented Oct 14, 2014

markelog commented Oct 14, 2014

markelog commented Oct 14, 2014

markelog commented Oct 14, 2014

mikesherov commented Oct 14, 2014

mikesherov commented Oct 14, 2014

markelog commented Oct 14, 2014