✨ Add aggregate score to cron JSON #1050
Conversation
laurentsimon
force-pushed
the
feat/cronagg
branch
2 times, most recently
from
1568149
to
f6c65a6
Compare
laurentsimon
requested review from
inferno-chromium,
naveensrinivasan and
oliverchang
| @@ -69,10 +69,21 @@ type jsonScorecardV2 struct { | |||
| Commit string `json:"commit"` | |||
| } | |||
|
|
|||
| type jsonFloatScore float64 | |||
|
|
|||
| func (s jsonFloatScore) MarshalJSON() ([]byte, error) { | |||
There was a problem hiding this comment.
Might skip this, just X.0 is fine for even ints.
There was a problem hiding this comment.
+1. Given how floats are represented I'm unsure this will work reliability in every case either.
There was a problem hiding this comment.
sure, can do that. I thought it was cleaner without the 0 if it's an int :-)
There was a problem hiding this comment.
why not keep everything float ?
There was a problem hiding this comment.
JSON does not understand the distinction between float and int in general. The schema we export (if users use it) has the field typed as number and not an integer.. so it's not an integer. Removing the .X was mostly esthetic when a human being looks at it. I'm fine to keep all X.Y... it may even be better: I can imagine users make it an int by mistake and only later realize it should be a float.
There was a problem hiding this comment.
Yes that is what i meant, keep everything float.
cron/format/json.go
Outdated
| @@ -128,6 +145,7 @@ func AsJSON2(r *pkg.ScorecardResult, showDetails bool, | |||
| }, | |||
| Date: r.Date.Format("2006-01-02"), | |||
| Metadata: r.Metadata, | |||
| AggScore: jsonFloatScore(score), | |||
There was a problem hiding this comment.
AggregateScore in full might be better to avoid confusion.
There was a problem hiding this comment.
sure. Note this is not the JSON field name, but just the structure field name. The JSON field is currently called just score. Is that ok?
| @@ -69,10 +69,21 @@ type jsonScorecardV2 struct { | |||
| Commit string `json:"commit"` | |||
| } | |||
|
|
|||
| type jsonFloatScore float64 | |||
|
|
|||
| func (s jsonFloatScore) MarshalJSON() ([]byte, error) { | |||
There was a problem hiding this comment.
+1. Given how floats are represented I'm unsure this will work reliability in every case either.
laurentsimon
force-pushed
the
feat/cronagg
branch
from
10f30e6
to
990bacb
Compare
| @@ -69,10 +69,21 @@ type jsonScorecardV2 struct { | |||
| Commit string `json:"commit"` | |||
| } | |||
|
|
|||
| type jsonFloatScore float64 | |||
|
|
|||
| func (s jsonFloatScore) MarshalJSON() ([]byte, error) { | |||
There was a problem hiding this comment.
Yes that is what i meant, keep everything float.
laurentsimon
force-pushed
the
feat/cronagg
branch
from
990bacb
to
7ba10c9
Compare
What kind of change does this PR introduce? (Bug fix, feature, docs update, ...)
feature
What is the current behavior? (You can also link to an open issue here)
see Need an aggregate risk score, ideally configurable (need for 3.0) #1040 and ✨ Add aggregated score #1046
What is the new behavior (if this is a feature change)?
see Need an aggregate risk score, ideally configurable (need for 3.0) #1040 and ✨ Add aggregated score #1046
Does this PR introduce a breaking change? (What changes might users need to make in their application due to this PR?)
no
Other information: