-
Notifications
You must be signed in to change notification settings - Fork 42
Add versioned outcomes schema #672
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
Conversation
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.
I'm fine with the bulk of this PR as-is. My only to blockers to merge are:
- do we need
data/json/decision_points.zip? - do we need
src/ssvc/gini_impurity.py
both of these seem out of place to me.
src/cvss_to_json.py
Outdated
| #user_interaction USER_INTERACTION_2 | ||
| print(mod, dp) | ||
| sdp = getattr(module, dp) | ||
| f = open(f"../data/json/decision_points/cvss/{dp.lower()}.json", "w") |
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.
Purely a syntactical note, not worth changing, just mentioning it since I noticed it here.
with open("...","w") as f:
f.write("foo")
does the same thing but you don't have to explicitly close() because the context manager handles it for you.
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.
will do with syntax...
|
Also note that we'll need to do a brief reconciliation of #674 with this one, just depends on merge order which one will need to change. |
This is an attempt to move towards Policy Explorer which requires Outcomes and resolves #589 so we have a schema for the outcome groups.
Here is what has happened
Mostly additions should not disturb any setup.