-
Notifications
You must be signed in to change notification settings - Fork 89
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
toolComponent change #1277
toolComponent change #1277
Conversation
src/Sarif/Schemata/sarif-schema.json
Outdated
"extensionIndex": { | ||
"description": "The index within the run.tool.extensions array of the tool component object which describes the plug-in or tool extension that produced the result.", | ||
"type": "integer", | ||
"minimum": -1 |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
minimum [](start = 11, length = 7)
and: "default": -1
#Resolved
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yes, did some clean up for minimum/default, please take a look at update.
In reply to: 255688316 [](ancestors = 255688316)
src/Sarif/Schemata/sarif-schema.json
Outdated
}, | ||
|
||
"extensions": { | ||
"description": "Tool extensions that contributed to or reconfigured the analysis tool run.", |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
run [](start = 96, length = 3)
"run" => "that was run" (for parallelism with description of "driver", and also I think it's clearer). #Closed
src/Sarif/Schemata/sarif-schema.json
Outdated
"properties": { | ||
|
||
"name": { | ||
"description": "The name of the tool.", | ||
"description": "The name of the tool driver or extension.", |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
driver or extension [](start = 47, length = 19)
How about "component" since that's what you use in the rest of the properties below. #Resolved
} | ||
}, | ||
|
||
"required": [ "driver" ] |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
"required": [ "driver" ] [](start = 6, length = 24)
Yes. #Closed
src/Sarif/Schemata/sarif-schema.json
Outdated
} | ||
}, | ||
|
||
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Extra blank line. #Closed
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Looks fine. I agree you should add |
Michael, didn't we talk about linking from a |
i put this in resultProvenance.extensionIndex. You don't populate this at all in cases where it came from the driver. Seems simpler for happy path production. ? In reply to: 462492896 [](ancestors = 462492896) |
yes! on it. ok, role, yes. In reply to: 462493928 [](ancestors = 462493928) |
In reply to: 462495693 [](ancestors = 462495693,462492896) |
src/Sarif/Schemata/sarif-schema.json
Outdated
@@ -518,7 +520,7 @@ | |||
"uriBaseId": { | |||
"description": "A string which indirectly specifies the absolute URI with respect to which a relative URI in the \"uri\" property is interpreted.", | |||
"type": "string" | |||
}, | |||
},` |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
` [](start = 10, length = 1)
typo #Closed
the default is -1, do you see a problem i need to fix? do you have a different set of standards for what belongs in provenance? In reply to: 462497387 [](ancestors = 462497387,462495693,462492896) |
No problem on the default. I misunderstood you. In reply to: 462498159 [](ancestors = 462498159,462497387,462495693,462492896) |
{ | ||
return this.NotificationsMetadata.HasAtLeastOneNonNullValue(); | ||
var toolComponent = new ToolComponent(); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
ToolComponent [](start = 36, length = 13)
Can you create a property bag directly? Do you need a toolComponent
to hold it? #Closed
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It is cumbersome to instantiate IDictionary<string, SerializedPropertyInfo> directly (I do it elsewhere in this change). This is the easier mechanism because you can use the SetProperty helper. Do you have other advice on creation? This specific changes retains the previous code pattern...
In reply to: 256107576 [](ancestors = 256107576)
@@ -177,7 +177,7 @@ public override Message VisitMessage(Message node) | |||
} | |||
|
|||
if (node.Text == null && | |||
_run.Tool?.GlobalMessageStrings?.TryGetValue(node.MessageId, out formatString) == true) | |||
_run.Tool.Driver.GlobalMessageStrings?.TryGetValue(node.MessageId, out formatString) == true) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
. [](start = 29, length = 1)
Why did you remove the null conditional operator? #Closed
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
check line 168, where it didn't exist. Tool.Driver is a required object. we could put an arg check at the top of the method
In reply to: 256108174 [](ancestors = 256108174)
@@ -36,6 +36,8 @@ private enum FieldIndex | |||
private CsvParser _parser; | |||
private List<Notification> _toolNotifications; | |||
|
|||
public override string ToolName { get { return "Semmle QL"; } } | |||
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
In all the other converters you just wrote
public override string ToolName => "Semmle QL";
``` #Closed
@@ -20,6 +20,8 @@ public CppCheckConverter() | |||
_strings = new CppCheckStrings(_nameTable); | |||
} | |||
|
|||
public override string ToolName => ToolFormat.CppCheck; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
CppCheck [](start = 54, length = 8)
I know we discussed, and I think we agreed, not to assume that the ToolFormat
would also make a good tool name. In most of the other converters, you're just hard-coding a string here ("CppCheck"
). #WontFix
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
in fact, most converters still use ToolFormat. I'm not going to invest more here, I spent hours on this change DRYing out test code, removing embedded literals. We can revisit this in the future if need be.
In reply to: 256111009 [](ancestors = 256111009)
@@ -43,5 +44,36 @@ public SarifLog RunTestCase(string inputData, string expectedResult, bool pretty | |||
|
|||
return log; | |||
} | |||
|
|||
private static SarifLog BuildToolSpecificEmptyLog() |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Nice. #Closed
int intPropertyValue = 42; | ||
|
||
string stringPropertyName = nameof(stringPropertyName); | ||
string stringPropetyValue = "'\"\\'"; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
stringPropetyValue [](start = 19, length = 18)
Propety
=> Property
#Closed
{ | ||
string expected = CreateCurrentV2SarifLogText( | ||
resultCount: 1, | ||
(log) => { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
log [](start = 17, length = 3)
I absolutely love this pattern. #Closed
"properties": { | ||
"tags": [ | ||
"singleTag" | ||
], |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
formatting. #Closed
"tags": [ "singleTag" ], | ||
"propertyKey": "propertyValue" | ||
}, | ||
"globalMessageStrings": { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
globalMessageStrings [](start = 9, length = 20)
In 11-28, these three properties didn't yet live in tool
, right? Global message strings and rules metadata lived in run.resources
, and notification metadata didn't yet exist. #Resolved
{ | ||
new Invocation | ||
{ | ||
ConfigurationNotifications = new List<Notification>(s_notifications) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
s_notifications [](start = 80, length = 15)
Hah. Really nice. #Closed
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This force argument always existed because some tests lie about their version: i.e., the profess to have a format that they don't. I cleaned that up and removed this arg. I'll remove the dead comment in the next PR. also, please note: the modified log arg can go away now. at this point, any transformation of a prerelease format entails a change (because a required property, tool, has been updated). In reply to: 462935363 [](ancestors = 462935363) Refers to: src/Sarif/Writers/PrereleaseCompatibilityTransformer.cs:42 in 2dc4f02. [](commit_id = 2dc4f02, deletion_comment = True) |
@lgolding can you sanity check the schema change for the new toolComponent object?
in re: oasis-tcs/sarif-spec#179