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

Schema changes #1140

Open
michaelcfanning opened this issue Nov 20, 2018 · 7 comments
Open

Schema changes #1140

michaelcfanning opened this issue Nov 20, 2018 · 7 comments

Comments

@michaelcfanning
Copy link
Member

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

@lgolding fyi

@ghost
Copy link

ghost commented Nov 20, 2018

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

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

ghost commented Nov 20, 2018

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

michaelcfanning commented Nov 20, 2018

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

ghost commented Nov 20, 2018

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

@ghost ghost closed this as completed Nov 20, 2018
@michaelcfanning
Copy link
Member Author

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.

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

No branches or pull requests

1 participant