MicrosoftPremier / VstsExtensions

Documentation and issue tracking for Microsoft Premier Services Visual Studio Team Services Extensions
MIT License
56 stars 14 forks source link

[BQC] Decorate PR with warnings #74

Open Bouke opened 4 years ago

Bouke commented 4 years ago

It would be great if this extension would be able to decorate the PR with newly added warnings. So the extension would add a comment on the exact line that triggered the warning. This allows for better feedback to the developer, as they will be quicker to spot the problem.

ReneSchumacher commented 4 years ago

Hi again!

This feature has been on our backlog for a while now. It would be easy to post comments for all warnings that we do actually understand (currently, this is only true for MSBuild related warnings that have all necessary information about the code location). However, it is not easy to just post comments for new or changed warnings, since we cannot really be sure if a warning is new or changed without running a deeper analysis on all the warnings.

Just imagine you have a file with 100 lines of code and there's a warning on lines 10, 20, and 25. If you now add a single line of code between the first and second warning, your warnings won't have changed but compared to the previous list, two warnings now have new line numbers (20 and 26 instead of 20 and 25). Yet, they're not new or changed warnings. Thus, we'd have to analyze the code changes between the baseline build and the current build to figure out the relationship between those changes and the changes we see in the warnings analysis.

I still don't have a good idea on how to handle this in a way that doesn't require getting different versions of the source code, comparing files etc. We have to keep in mind that the task must run quickly so it doesn't interfere too much with CI workflows (no one wants slow feedback from a CI build).

Bouke commented 4 years ago

For some use-cases it might be OK to have simple heuristics to determine whether a warning was triggered by the new build. So such a simple heuristic would do an exact comparison (diff) of the warnings, and assume all new errors to be newly introduced. This will cause false positives, that's for sure.

When comparing source code (e.g. through "git blame"), we shouldn't just ignore new warnings on code lines that haven't changed. We might deprecate some api ([Obsolete]), which would trigger warnings throughout the code-base (on old code), and those warnings are of interest.

Maybe a what more compelling heuristic would be to assume a warning as existing when:

I suspect that such a heuristic will be rather computationally expensive, and the more simplistic heuristic might suffice for some or even most users. This is all assuming BQC understands those warnings of course.

ReneSchumacher commented 4 years ago

Hi @Bouke,

we finally implemented a first heuristic analysis of warnings and display more detailed information in the warning statistics (i.e., summary section of the task). We'll give this some time to generate feedback from users and, assuming most are happy with the output, will start using that information in PR comments. Since you seem to be using the warning policy yourself, it'd be great to get some feedback from you, esp. if you believe something is off with the analysis.