Provide filtering hook for errors so they can be suppressed by plugins

[lawnsea]

I recently implemented a JSCS pre-commit hook on a large codebase. Unfortunately, there are thousands of existing style violations. We have filtered the hook to staged files, but this can still result in a situation where an engineer makes a stylistically-correct change to a file and has their commit aborted because of pre-existing style violations in the file they modified.

The solution to this problem that I have come up with is to filter JSCS's results against lines modified in the staged file. I have a couple of questions:

1. Is there a way to do this with the existing code that I have overlooked?
2. If not, how should I proceed?

I have a strawman that seems like a bad/workable idea: reporters are passed the actual collection of style errors, so I could write a reporter that filters that against the currently staged changeset.

Imma do this, but I wonder if it would be better to teach JSCS a filtering hook instead.

What do y'all think?

[mdevils]

You can have a simple pre-commit hook to run JSCS against changed files. This would solve your problem if I got you right.

[lawnsea]

As I mentioned above, we have a pre-commit hook that runs JSCS against staged files only. That part works just fine.

The problem is this: suppose I have a file foo/bar.js, which is 100 lines of JS. There are pre-existing style errors on lines 1, 2, 23, and 99. I make a stylistically correct edit to line 42 and then do git add foo/bar.js and git commit.

The commit will fail when the pre-commit hook runs, because JSCS will report four style errors. What we want to do is ignore errors on lines that are not added or modified in the staged commit.

In the example, it would be easy to fix the other errors and try the commit again. Unfortunately, in some files there are literally hundreds of existing style errors. We are working to clean them up, but in the interim we don't want to spam developers with a ton of style errors unrelated to their changes.

I've implemented a reporter that accomplishes the above by splicing the unrelated errors out of the errorCollection error arrays. It then passes the mutated collection to the console reporter:

module.exports = function (errorsCollection) {
  errorsCollection.forEach(function (errors) {
    if (!errors.isEmpty()) {
      var errorList = errors.getErrorList();
      var i = 0;

      while (i < errorList.length) {
        // is this line modified in the currently staged changes?
        if (!isModified(errors.getFilename(), errorList[i].line)) {
          errorList.splice(i, 1);
        } else {
          i++;
        }
      }
    }
  });

  // delegate to the console reporter
  require('jscs/lib/reporters/console')(errorsCollection);
};

This works, but it seems hacky to me, since it depends on the implementation detail that reporters receive the same errorCollection that is used by lib/cli.js to determine whether to exit with an error.

So, my question is: do you plan to keep this implementation detail in future versions of JSCS, and, if not, would you accept a pull request that adds a filter hook to lib/cli.js to support this use case?

[mikesherov]

@lawnsea thanks for contributing. IMO, when an engineer cleans up a file, they should clean up the whole thing. This resolves the problem much quicker and gets to a nicely styled codebase quickly.

However, I understand your situation. I will be adding a hook to filter errors shortly, and you'll be able to guarantee that it will happen right before errors are passed to reporters and you'll be able to hook into it.

[mrjoelkemp]

@mikesherov To clarify the potential implementation, are you talking about adding another cli option that allows a user to supply a path to a filters module (that exports a function that takes in the error list) that we require in and call with the errors before passing it to the reporter?

[mikesherov]

@mrjoelkemp yes, exactly. The idea is to split filtering out from reporting.

The only built in filter we'd have is the maxError option, which is also slated for 1.7.

[mrjoelkemp]

@mikesherov So if there's a custom filter hook, then use it and supply the return value of the hook to the reporter. Separately, if maxError is set to X (specifics of how to set it addressed in #238 (comment)), then slice off the first X items and pass that to the reporter (not that naive) halt further string checking once the maximum number of errors have been reached.

If the above specification is the expected flow for filters, the implementation seems pretty straightforward. I can tackle both this and maxErr #238 (pending the specifics on setting the value) and have a PR ready for review very soon.

[mikesherov]

@mrjoelkemp, we need to have maxError as a separate filter from any user supplied filter. Perhaps an array of filters seems the best.

[mrjoelkemp]

1. Should we support a user providing a path to a directory containing various filters? Or solely accept a path to a single filter (they could just wire up multiple filters to be used within a single filter module)?
   • If we don't support a directory of filters, then I don't really see the need for an array of filters (I'm not entirely sure that a maxErrors belongs in the filter list given its far reach outside of the errors object)
2. A "filter" should be a function (whose context is a lib/errors object) that accepts the following error literal and returns a boolean indicating whether or not the error should be subsequently added to the error object's _errorList.
3. Upon calling errors.add within rule checking, we run the generated error literal through all of the filters. If the error passes through (all filters return true indicating that a rule should be added), then proceed with adding the error to the _errorList.

Are we in agreement of what a filter should do, has access to, and when it's invoked?

[mikesherov]

1. We should accept a file path to a filter for right now. In the future, we could extend this to accept a directory too with no API change needed. 
2. I was envisioning a filter as 2 parted: 1. a function for filename filtering, ran before any checking at all, that reduces the list of files to check. 2. A post processor function, right before reporting, that filters by location in file and message. That seems the most straightforward way, IMO. 
3. No need to do this if my second suggestion is used. 

However, I think either method is fine. 
Mike Sherov

On Tue, Sep 30, 2014 at 11:37 PM, Joel Kemp notifications@github.com
wrote:

1. Should we support a user providing a path to a directory containing various filters? Or solely accept a path to a single filter (they could just wire up multiple filters to be used within a single filter module)?
   • If we don't support a directory of filters, then I don't really see the need for an array of filters (I'm not entirely sure that a maxErrors belongs in the filter list given its far reach outside of the errors object)
2. A "filter" should be a function (whose context is a lib/errors object) that accepts the following error literal and returns a boolean indicating whether or not the error should be subsequently added to the error object's _errorList.

3. Upon calling errors.add within rule checking, we run the generated error literal through all of the filters. If the error passes through (all filters return true indicating that a rule should be added), then proceed with adding the error to the _errorList.
   Are we in agreement of what a filter should do, has access to, and when it's invoked?

   Reply to this email directly or view it on GitHub:
   Provide filtering hook for errors so they can be suppressed by plugins #335 (comment)

[mrjoelkemp]

Thanks, Mike.

I wanted to avoid the case of post-filtering since maxerr would cap the
number of errors - yielding an incorrect number of filtered errors.

It would be strange to see 2 errors reported post-filtering when there are
more errors that weren't part of the first unfiltered batch of 50 errors.
By filtering before adding an error, we give the user a relevant set of 50
errors.
On Oct 1, 2014 7:20 AM, "Mike Sherov" notifications@github.com wrote:

1. We should accept a file path to a filter for right now. In the future,
   we could extend this to accept a directory too with no API change needed.
2. I was envisioning a filter as 2 parted: 1. a function for filename
   filtering, ran before any checking at all, that reduces the list of files
   to check. 2. A post processor function, right before reporting, that
   filters by location in file and message. That seems the most
   straightforward way, IMO.
3. No need to do this if my second suggestion is used.

However, I think either method is fine.
Mike Sherov

On Tue, Sep 30, 2014 at 11:37 PM, Joel Kemp notifications@github.com
wrote:

1. Should we support a user providing a path to a directory containing
   various filters? Or solely accept a path to a single filter (they could
   just wire up multiple filters to be used within a single filter module)?
2. If we don't support a directory of filters, then I don't really see
   the need for an array of filters (I'm not entirely sure that a maxErrors
   belongs in the filter list given its far reach outside of the errors
   object)
3. A "filter" should be a function (whose context is a lib/errors
   object) that accepts the following error literal
   and returns a boolean indicating whether or not the error should be
   subsequently added to the error object's _errorList.

4. Upon calling errors.add within rule checking, we run the generated
   error literal
   through all of the filters. If the error passes through (all filters return
   true indicating that a rule should be added), then proceed with adding
   the error to the _errorList.
   Are we in agreement of what a filter should do, has access to, and when
   it's invoked?

   Reply to this email directly or view it on GitHub:
   Provide filtering hook for errors so they can be suppressed by plugins #335 (comment)

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

[mikesherov]

It would be strange to see 2 errors reported post-filtering when there are
more errors that weren't part of the first unfiltered batch of 50 errors.
By filtering before adding an error, we give the user a relevant set of 50
errors.

Makes sense.

[mikesherov]

Still, a 2 part filter makes sense:

1. before even parsing files, ability to filter out files to parse.
2. a second set of filters that are applied inside errors.add, as you suggested.

[mrjoelkemp]

@mikesherov @markelog I plan to implement the following:

// sample config
{
  filenameFilter: 'path/to/my/filter.js',
  errorFilter: 'path/to/my/filter.js'
}

Similarly, there will be cli options:

--error-filter <filepath>
--filename-filter <filepath>

In regards to the config, I know that there are various possible forms of a jscsrc. Should I support a function object passed as the value for filenameFilter and errorFilter (similar to Murat's plugin architecture)? Should that be in a follow up commit, if considered?

I'll open a PR this week.

[markelog]

I don't think we need this kind of functionality, this could be solved in number of ways, not to mentioned that this is pretty much an edge case, plus i hope autoformating would fix it all.

[mrjoelkemp]

Happy you objected, Oleg.

The filenameFilter actually feels duplicative of excludeFiles.

The errorFilter would definitely solve this case, but I can't think of another practical use case – aside from generally allowing users to enrich error messages.

Agreed that autoformatting would fix the root of this problem.

@lawnsea What are your thoughts on waiting until we have an autoformatter to take care of fixing all errors within a file?

[lawnsea]

I'm not sure that an autoformatter would exactly fix our use case, as we would want to do any autoformatting in a separate PR. That said, we have the solution I originally outlined in place now, so we're in no rush.

I have, however, received several inquiries about implementing my hack in other codebases, so there may be more need for this feature than is apparent from this thread.

[mikesherov]

I agree with @lawnsea.

There only needs to be one "filter", which should be a path to a filter module, which receives the errors, and can manually inspect them and discard them however it sees fit.

This would allow precommit hooks, CI tools, etc. inspect certain files in a way that 'excludeFiles' can not. Even with autoformat, and with custom reporters, it seems like a good idea to have filtering as a separate, user definable step.

[mikesherov]

@lawnsea, what does your solution look like? I'll love to have that help inform the direction we go in with this.