Ability to deactivate Auto-refactor on code portions

[sauthieg]

I would like to tell AR to ignore some portion of my code. Let say that these portions contain some false-positive matches or are really ugly but I don't want to touch them right now :)

Other analyzers (Sonar, Checkstyle) are using a comment based convention to ignore code snippets:

Could be for a single line:

        // Prints the exchange if required
        if (captureExchange) {
            writer.println("Exchange's content as JSON (without request/response):");
            captureExchangeAsJson(writer, exchange); // NOAR
        }

or for a whole block:

        // Prints the exchange if required
        // @autorefactor:off
        if (captureExchange) {
            writer.println("Exchange's content as JSON (without request/response):");
            captureExchangeAsJson(writer, exchange);
        }
        // @autorefactor:on

[JnRouvignac]

I think the scope of such a maker should as limited as possible. For example, every refactoring rule should have a reference number assigned to it, then this is what would have to be put into the marker to avoid fully disabling AutoRefactor for a piece of code.

Of course, AutoRefactor should provide an easy mean to insert such markers. Possibly via #101 where there would be a "Ignore forever" or "Insert ignore marker" tickbox, in addition to the simpler "Ignore" tickbox?

[sauthieg]

It should be limited in the surface of code ignored (just a few lines), but that's up to the user to decide. I'm not convinced that we need to just say don't apply that rule, and that one and this other one. From what I've seen in other code analyzers, the marker just means 'disable everything'.

Without saying that this will become hard to write a marker if we have to exclude a set of rules (inclusive or exclusive ?).

I like the idea to automatically add a marker that will "protect" the right snippet of code. I'm sure that it will help because of that kind of wrong (I think) markers:

// Prints the exchange if required
// @autorefactor:off
 if (captureExchange) {
  writer.println("Exchange's content as JSON (without request/response):");
  // @autorefactor:on
  captureExchangeAsJson(writer, exchange);
}

[JnRouvignac]

I don't like the "disable everything" because it is very likely this can hide other problems that could be interesting to fix. I suspect other tools do it this way because it is a very attractive painless solution compared to correctly documenting what the tool does.

That being said, a "disable all" approach would be a very interesting and acceptable first step. It's just that it should not be the last step.

[sauthieg]

:)

I think that this depends on the surface of code ignored. For a 3 lines block to be ignored, this is very likely that there is only 1 rules that will apply.

If you ignore a whole class, then this is another story ;)

[JnRouvignac]

We'll certainly have to toy around with such feature. It is unclear to me how some refactoring rule should be marked to be ignored.

Since some rules can impact big if statements (for example), the disabling feature could span over as many lines as the if statement. This could hide many things with the "disable all" approach.