-
Notifications
You must be signed in to change notification settings - Fork 14
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
[BQC] Decorate PR with warnings #74
Comments
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). |
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 ( 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. |
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. |
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.
The text was updated successfully, but these errors were encountered: