runLinter: exit 1 or 0 when declarations fail linting?

Stream: batteries

[Thomas Murrills]

I noticed that if any declarations fail linting, we exit runLinter with code 1, and radar fails entirely.

I'm not sure this is actually a radar issue; perhaps runLinter should not be exiting with code 1 when declarations fail linting. After all, the program itself succeeded; and for radar to handle it, it would have to capture every exit 1 in runLinter, even less intentional ones.

I'm not quite sure what best practice is here. Perhaps a warningAsError config flag, like lean has, which determines whether we exit with 0 or 1 on linting warnings? (And should linting warnings be printed to stdout or stderr?)

[Chris Henson]

Could you point out a radar run affected by this? I think I'm missing some context about when this happens.

My initial thought (that you've probably already considered) is that downstream breakage of CI needs to be considered/coordinated for this, otherwise anything that uses this as its lint driver will begin to silently allow failures.

[Eric Wieser]

I don't think radar should be benchmarking runLinter at all if it fails

[Thomas Murrills]

Chris Henson said:

Could you point out a radar run affected by this? I think I'm missing some context about when this happens.

Sure! It happens when you either introduce declarations that violate a linter or modify/introduce linters in a way that introduces violations. Consider e.g. this bench.

Chris Henson said:

My initial thought (that you've probably already considered) is that downstream breakage of CI needs to be considered/coordinated for this, otherwise anything that uses this as its lint driver will begin to silently allow failures.

Yeah, true; maybe an opt-in flag to not exit 1 would be best?

[Thomas Murrills]

Eric Wieser said:

I don't think radar should be benchmarking the linter at all if it fails

The issue is that the whole run fails, so you don’t get information on the build either.

Maybe that means radar should split these into two separate pieces of the bench somehow, so that the linter failing doesn’t make radar fail?

(I do think the notion of “runLinter succeeded in reporting linting warnings, therefore exit 0” is probably a useful mode in principle, but yeah, maybe it’s not the right approach here after all…)

[Chris Henson]

I see. I agree with you that it would be nice to split these. Even for this very particular case though, I don't think a zero exit code would be great.