Skip to content
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

Open
Bouke opened this issue Feb 7, 2020 · 3 comments
Open

[BQC] Decorate PR with warnings #74

Bouke opened this issue Feb 7, 2020 · 3 comments
Assignees

Comments

@Bouke
Copy link

Bouke commented Feb 7, 2020

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 ReneSchumacher self-assigned this Feb 7, 2020
@ReneSchumacher
Copy link
Member

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
Copy link
Author

Bouke commented Feb 11, 2020

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:

  • the location of the warning is close to the previous location
  • and from source control we can determine that this code didn't change since the previous build

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
Copy link
Member

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.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

No branches or pull requests

2 participants