-
Notifications
You must be signed in to change notification settings - Fork 36
Keep use of interfaces in readme simple and CVSS version agnostic. #73
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
README.rst
Outdated
| print(vector) | ||
| print(c.base_score) | ||
| print(c.severity) | ||
| print(c.severity) # Note this is for CVSS4. For consistent interface between all versions, use .severities() |
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 think we should make all the examples consistent to avoid any misunderstanding about why CVSS4 includes something that CVSS3 or CVSS2 do not, and so on:
vector = 'AV:L/AC:L/Au:M/C:N/I:P/A:C/E:U/RL:W/RC:ND/CDP:L/TD:H/CR:ND/IR:ND/AR:M'
c = CVSS2(vector)
print(vector)
print(c.clean_vector())
print(c.scores())
print(c.severities())
print()
vector = 'CVSS:3.0/S:C/C:H/I:H/A:N/AV:P/AC:H/PR:H/UI:R/E:H/RL:O/RC:R/CR:H/IR:X/AR:X/MAC:H/MPR:X/MUI:X/MC:L/MA:X'
c = CVSS3(vector)
print(vector)
print(c.clean_vector())
print(c.scores())
print(c.severities())
print()
vector = 'CVSS:4.0/AV:N/AC:L/AT:N/PR:N/UI:N/VC:H/VI:H/VA:H/SC:H/SI:H/SA:N'
c = CVSS4(vector)
print(vector)
print(c.clean_vector())
print(c.scores())
print(c.severities())
print()
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 agree with both of you. I would actually like it this way:
...
c = CVSS4(vector)
print(vector)
print(c.clean_vector())
print(c.scores())
print(c.severities())
# Since CVSS4 has only one score and severity, attributes can be used instead if prefered
print(c.base_score)
print(c.severity)
...
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.
@skontar cvss2.base_score and cvss3.base_score work as well (but cvss2.severity and cvss3.severity do not), so it might be better just to omit it, but no strong preference
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.
Thank you all for review. Went with Jitka's suggestion. Subjective reasons:
- Simple.
- Interface usage kept consistent.
- Nonconfusing for people not familiar with the code.
- The version-specific property interfaces can be discovered by reading the implementation. For those who don't do that, information about that can be superfluous, unasked for, confusing. And those who do it discover it.
Is that fine @skontar ? (I know, bikeshedding at this point 😺 )
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.
That is also fine with me. Maybe just add a comment that it is expected in v4 to return tuple of just one value due to v4 spec?
ca849e1 to
90d9ac8
Compare
90d9ac8 to
dfe9e91
Compare
jobselko
left a comment
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.
The PR title could also be updated to better reflect the changes, but otherwise, LGTM.
Closes #71
It might seem as a WONTFIX issue, but to be frank, I was confused for 10 minutes about that too.