Only first occurrence of warning per line is reported

[wojciechczerniak]

There are two identical mistakes in this line: color rgba(0, 0, .7, .5) and only one of them is reported. If you fix a warning the next mistake will be found and that's ok, eventually all of them will be fixed. But having 150 warnings and after fixing few of them still having 150 reported is confusing. And it would look good to highlight all of them in IDE.

[wojciechczerniak]

Note to self: Found a way to report all occurrences of regExp. Now a DRY method would be nice. Maybe in v2 this.reporter from #269 can handle this.

[SimenB]

This should be solved by the rules themselves. Either call some sort of report function multiple times, or return an array.
Eslint uses a method called x amount of times.

@wojciechczerniak Any preference on which method we do this?

[wojciechczerniak]

I agree. Only rule will know if its violations can occur multiple times within a line. Trying to do it outside wouldn't be as elegant solution (separation) and had a performance impact running double checks where it's not necessary.

In #356 we wanted to cleanup inner calls to reporter function and return an object with violation. Maybe this should be an array of objects? Or only an array if multiple violations found and single object otherwise? We can always detect that later and act accordingly (concat or push).

[SimenB]

Whether the rules return an object/array of objects or invoke a callback doesn't really matter to me (single object made into array is as easy as messages = Array.isArray(messages) ? messages : [messages];)

It will have to be sync anyways, so maybe return is clearer? On the other hand, that means the rules must manage som internal state in order to return everything, instead of just invoking a function every time they find anything.

Both are easily testable (passing in a spy if we go for functions).

I'm leaning slightly towards callbacks, as that moves all responsibility for state into core and out of the rules.

Any arguments either way?

Should we close this issue, and add it as a point under #356? It concerns the contract rules must conform to, might be just as easy to just do it all in one fell swoop there.

[wojciechczerniak]

We can avoid keeping state within smaller rules if they can report every time they encounter violation, but can we with more complicated checks?

There is one rule duplicates which even had own properties on global state. This breaks separation between rules and core. It's a choice between "state in rule" or "rule in core" then?

I also don't see much difference between those two. More how we see code structure and flow inside a rule. Returning something from function is strong indication that check is finished. With callbacks you can return them or not. Do check has to indicate it's finished with return anyway or just stop calling callbacks? If they return then what? true / false as it does now? We wanted to avoid that.

Rule gets a line as argument. I think that keeping a state for that line until it's done with it isn't that big deal. Isn't this what it supposed to do, lint the line and return report? Anything it does inside is only its own concern.

Yup, this should become a point under #356 when we finally decide which way to go 😉

[SimenB]

My thoughts: If we go for callbacks, we don't care if the rule returns or not.
Again, the rules HAVE to report synchronously anyways (this is to better support programmatic usage, even IO should be synchronous), so the callbacks won't be async.

If a rule is advanced (like duplicates), then it can have that state sure, didn't think of that use case. But rules shouldn't have to keep state concerning what they have reported on earlier (or will report on in the future).

But I don't feel strongly about this, they can return and we'll concat on it.

Could experiment after the other parts of #356 is done? After they don't get to use this anymore, it'll be easier to experiment with what API makes the most sense.

But if we in the future open for custom rules, we want them to be able to be implemented as cleanly as possible. That's really the only thing I care about. Move as much complexity as possible into core, and try to keep rules as stateless as possible

[SimenB]

A part of this issue should be to identify which rules potentially should report multiple times. Not necessarily make them report multiple times, but at least create the list so we can check of items as they get fixed.

[SimenB]

I'm gonna remove this from the 2.0 effort, to minimize the wait for it.

Isn't really breaking when it's fixed either, IMO