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

Optimize partitioned logs by removing not relevant rules #2369

Merged
merged 6 commits into from
Jun 21, 2021

Conversation

yongyan-gh
Copy link
Collaborator

No description provided.

@lgtm-com
Copy link

lgtm-com bot commented Jun 16, 2021

This pull request introduces 1 alert when merging 55b1929 into cfb4f9b - view on LGTM.com

new alerts:

  • 1 for Useless assignment to local variable

return null;
}

var referencedRulesByIndex = new Dictionary<ReportingDescriptor, int>();
Copy link
Collaborator

@eddynaka eddynaka Jun 16, 2021

Choose a reason for hiding this comment

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

()

Check if you can use ReportingDescriptorEqualityComparer #Closed

var referencedRulesByIndex = new Dictionary<ReportingDescriptor, int>();
int index = 0;

foreach (Result result in results)
Copy link
Collaborator

@eddynaka eddynaka Jun 16, 2021

Choose a reason for hiding this comment

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

result

check if u have access to the run here, if yes, there is a GetRule method in the result #Closed

@@ -37,6 +37,11 @@ public static IEnumerable<SarifLog> Split(this SarifLog sarifLog, SplittingStrat
partitionFunction = (result) => result.Run.GetHashCode().ToString();
break;
}
case SplittingStrategy.PerRunPerTarget:
{
partitionFunction = (result) => result.Locations.FirstOrDefault().PhysicalLocation.ArtifactLocation.Uri.OriginalString;
Copy link
Collaborator

Choose a reason for hiding this comment

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

result.Locations

a result without location = Locations = null or Locations = []?

either way, please, add one test for that

@@ -177,8 +177,6 @@ public virtual IReadOnlyList<SarifLog> SplitLogFile(SarifLog sarifLog)
this.FilingResult = FilingResult.None;
this.FiledWorkItems = new List<WorkItemModel>();

sarifLog = sarifLog ?? throw new ArgumentNullException(nameof(sarifLog));
Copy link
Collaborator

@eddynaka eddynaka Jun 16, 2021

Choose a reason for hiding this comment

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

sarifLog = sarifLog

nice change :) #ByDesign

@yongyan-gh yongyan-gh marked this pull request as ready for review June 18, 2021 19:43
@eddynaka eddynaka enabled auto-merge (squash) June 21, 2021 22:57
Copy link
Collaborator

@eddynaka eddynaka left a comment

Choose a reason for hiding this comment

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

:shipit:

@eddynaka eddynaka merged commit 744db99 into main Jun 21, 2021
@eddynaka eddynaka deleted the users/yongyan-gh/fileworkitems branch June 21, 2021 22:58
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.

None yet

2 participants