-
Notifications
You must be signed in to change notification settings - Fork 160
Implement ScoreCard's model and backend #1498
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
f642513
to
0ed5932
Compare
@sortie this has been updated with the latest 2.0 changes, and can be reviewed again |
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.
lgtm
app/lib/scorecard/backend.dart
Outdated
return null; | ||
} | ||
all.sort((a, b) => | ||
isNewer(a.semanticRuntimeVersion, b.semanticRuntimeVersion) ? -1 : 1); |
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.
It's faster to search for the latest rather than sort and then take the last.
app/lib/scorecard/backend.dart
Outdated
ScoreCardBackend get scoreCardBackend => | ||
ss.lookup(#_scorecard_backend) as ScoreCardBackend; | ||
|
||
class ScoreCardBackend { |
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.
Where is the documentation on score cards? Can add you add some words on the data model and the operations done here?
Should there be tests for this code and score cards?
app/lib/scorecard/backend.dart
Outdated
Future<Map<String, ReportData>> loadReports( | ||
String packageName, String packageVersion, | ||
{List<String> reportTypes}) async { | ||
reportTypes ??= [ReportType.pana, ReportType.dartdoc]; |
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.
These are static const, can you set a default parameter value for reportTypes instead, if the list can be const?
static const String failed = 'failed'; | ||
static const String aborted = 'aborted'; | ||
} | ||
|
||
/// Summary of various reports for a given PackageVersion. |
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.
This documentation seems a little sparse?
@CompatibleStringListProperty() | ||
List<String> platformTags; | ||
|
||
/// The flags for the package, version or analysis. |
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.
Maybe list a few?
app/lib/scorecard/models.dart
Outdated
@CompatibleStringListProperty() | ||
List<String> platformTags; | ||
|
||
final String platformReason; |
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.
What is this?
…emoved, flags are handled as a List of values.
0ed5932
to
0c37605
Compare
I've updated the documentation, but looking at it after a month or so, I've found the field duplicates in the |
lgtm |
Many steps closer to #1352
ScoreCardData
will replaceAnalysisExtract
with efficient storage and caching.ScoreCard
stores a lot of relevant information that is required for replacingJob
scheduling.ScoreCardReport
will replaceAnalysis
and at the same time will allowdartdoc
and other reports reports to be stored and handled the same way.