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

SLVS-1679 Enable issue streaming for SLCore analysis #5876

Merged

Conversation

georgii-borovinskikh-sonarsource
Copy link
Contributor

@georgii-borovinskikh-sonarsource georgii-borovinskikh-sonarsource commented Dec 5, 2024

@@ -92,12 +88,10 @@ public void Accept(string path, IEnumerable<IAnalysisIssue> issues)

Debug.Assert(issues.All(IsIssueFileLevelOrInAnalysisSnapshot), "Not all reported issues could be mapped to the analysis snapshot");

var newIssues = issues
allIssues = issues
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Previously, all of the different 'analyzers' were publishing to the same issue consumer, so it needed to be accumulating. Now, we only have one analyzer, which posts all issues for all languages for the file, meaning we no longer need to accumulate

@@ -78,12 +78,6 @@ private bool IsValid<T>(RaiseFindingParams<T> parameters) where T : RaisedFindin
return false;
}

if (parameters.isIntermediatePublication)
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

With the change to IssueConsumer, we can stop ignoring the intermediate events. This allows us to have issue streaming from SLCore

Copy link

sonarqubecloud bot commented Dec 5, 2024

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Some minor feedback

@@ -142,7 +142,7 @@ private void HandleAnalysisIssue(Message message)
// TextBufferIssueTracker when the file was closed, but the TextBufferIssueTracker will
// still exist and handle the call.
// todo https://sonarsource.atlassian.net/browse/SLVS-1661

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Please remove the TODO

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Why? This links to the cleanup task

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

You added it after you commented out the line 145 in this commit: 30e2dd5

Then you added back the line, so I assumed the TODO also has to be removed.

If that is not the case, then consider explaining what has to be done in the line and update the task SLVS-1661 to make sure the TODO will be removed

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The whole class needs to be removed, I reverted the line change because I didn't want to comment out the tests

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Then, just update the corresponding Jira task, and everything should be fine. 👍

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

Successfully merging this pull request may close these issues.

2 participants