search

microsoft / sarif-sdk

.NET code and supporting files for working with the 'Static Analysis Results Interchange Format' (SARIF, see https://github.com/oasis-tcs/sarif-spec)
Other
193 stars 93 forks source link

Schema changes #1140

Open michaelcfanning opened 5 years ago

michaelcfanning commented 5 years ago

1) mark result,message as required 2) order rule.configuration.defaultLevel and result.level enum values so that all shared members exist at the same ordinal value. this will allow configuration default level to be directly castable to result level in the SDK.

michaelcfanning commented 5 years ago

@lgolding fyi

ghost commented 5 years ago

@michaelcfanning #1 is covered by oasis-tcs/sarif-spec#283.

As for #2, it makes me uncomfortable to build into the SDK an assumption about the format that might not always be true. When our great-grand-editors insert a new value into one of those enumerations that breaks the correspondence, SDK code -- or even client code! -- that performs that cast will break mysteriously. Even if our great-grand-SDK-maintainers understand this dependency and account for the change, they won't be able to fix client code.

michaelcfanning commented 5 years ago

Yes. We should and will do something more bullet-proof in the SDK. I would still prefer to order these similarly in the schema.

… it may be that we need to simply separate these concepts as you suggested we consider in an offline call.

ghost commented 5 years ago

Yes, the separation of concepts might help. But even if we kept the existing enumerations, and even if we adjusted the ordering as you suggest, I'd never write SDK code that relied on it, nor recommend that anyone write client code that relied on it (which was your stated rationale for requesting the reordering).

The rationale for the current ordering is that it's more or less in increasing order of "severity" (if you consider "pass" to be a really low severity, and "notApplicable" even lower :-)). Changing the order to (for example)

[ "note", "warning", "error", "pass", "notApplicable", "open" ]

seems at first glance to be random (IMO), but it does add weight to the argument that we're really mixing two concepts here.

michaelcfanning commented 5 years ago

As mentioned, I agree with your position on the SDK. I yield. Let's just proceed with adding result.message as required. I like your rationale around current enum value ordering.

ghost commented 5 years ago

Ok, closing this issue, then, as result.message is covered by oasis-tcs/sarif-spec#283.

michaelcfanning commented 5 years ago

Let's leave this issue open until the SARIF SDK schema is fixed. I suggest that we leave the OASIS issue open until the OASIS copy is fixed. I realize this creates some issue duplication.