-
Notifications
You must be signed in to change notification settings - Fork 152
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
Calculate dartdoc coverage score and fix scorecard-model fields. #1711
Changes from all commits
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -3,6 +3,7 @@ | |
// BSD-style license that can be found in the LICENSE file. | ||
|
||
import 'dart:async'; | ||
import 'dart:convert' as convert; | ||
import 'dart:io'; | ||
|
||
import 'package:logging/logging.dart'; | ||
|
@@ -26,6 +27,7 @@ import '../shared/versions.dart' as versions; | |
import 'backend.dart'; | ||
import 'customization.dart'; | ||
import 'models.dart'; | ||
import 'pub_dartdoc_data.dart'; | ||
|
||
final Logger _logger = new Logger('pub.dartdoc.runner'); | ||
final Uuid _uuid = new Uuid(); | ||
|
@@ -34,9 +36,14 @@ const statusFilePath = 'status.json'; | |
const _archiveFilePath = 'package.tar.gz'; | ||
const _buildLogFilePath = 'log.txt'; | ||
const _packageTimeout = const Duration(minutes: 10); | ||
const _pubDataFileName = 'pub-data.json'; | ||
const _sdkTimeout = const Duration(minutes: 20); | ||
final Duration _twoYears = const Duration(days: 2 * 365); | ||
|
||
// We'll emit a suggestion and apply score penalty only if the coverage is below | ||
// this value. | ||
const _coverageEmitThreshold = 0.1; | ||
|
||
final _pkgPubDartdocDir = | ||
Platform.script.resolve('../../pkg/pub_dartdoc').toFilePath(); | ||
|
||
|
@@ -73,7 +80,7 @@ class DartdocJobProcessor extends JobProcessor { | |
timeout: _sdkTimeout, | ||
); | ||
|
||
final pubDataFile = new File(p.join(outputDir, 'pub-data.json')); | ||
final pubDataFile = new File(p.join(outputDir, _pubDataFileName)); | ||
final hasPubData = await pubDataFile.exists(); | ||
final isOk = pr.exitCode == 0 && hasPubData; | ||
if (!isOk) { | ||
|
@@ -124,7 +131,8 @@ class DartdocJobProcessor extends JobProcessor { | |
bool hasContent = false; | ||
|
||
String reportStatus = ReportStatus.failed; | ||
final suggestions = <Suggestion>[]; | ||
final healthSuggestions = <Suggestion>[]; | ||
final maintenanceSuggestions = <Suggestion>[]; | ||
try { | ||
final pkgDir = await downloadPackage(job.packageName, job.packageVersion); | ||
if (pkgDir == null) { | ||
|
@@ -197,17 +205,52 @@ class DartdocJobProcessor extends JobProcessor { | |
await toolEnvRef.release(); | ||
} | ||
|
||
if (!hasContent) { | ||
suggestions.add(getDartdocRunFailedSuggestion()); | ||
double coverageScore = 0.0; | ||
final dartdocData = await _loadPubDartdocData(outputDir); | ||
if (hasContent && dartdocData != null) { | ||
final total = dartdocData.apiElements.length; | ||
final documented = dartdocData.apiElements | ||
.where((elem) => | ||
elem.documentation != null && | ||
elem.documentation.isNotEmpty && | ||
elem.documentation.trim().length >= 5) | ||
.length; | ||
if (total == documented) { | ||
// this also handles total == 0 | ||
coverageScore = 1.0; | ||
} else { | ||
coverageScore = documented / total; | ||
} | ||
|
||
if (coverageScore < _coverageEmitThreshold) { | ||
final level = coverageScore < 0.2 | ||
? SuggestionLevel.warning | ||
: SuggestionLevel.hint; | ||
final undocumented = total - documented; | ||
healthSuggestions.add( | ||
new Suggestion( | ||
'dartdoc.coverage', // TODO: extract as const in pana | ||
level, | ||
'Document public APIs', | ||
'$undocumented out of $total API elements (library, class, field ' | ||
'or method) have no adequate dartdoc content. Good documentation ' | ||
'improves code readability and discoverability through search.', | ||
score: (1.0 - coverageScore) * 10.0), | ||
); | ||
} | ||
} else { | ||
maintenanceSuggestions.add(getDartdocRunFailedSuggestion()); | ||
} | ||
// TODO: calculate coverage score | ||
await scoreCardBackend.updateReport( | ||
job.packageName, | ||
job.packageVersion, | ||
new DartdocReport( | ||
reportStatus: reportStatus, | ||
coverageScore: hasContent ? 1.0 : 0.0, | ||
suggestions: suggestions.isEmpty ? null : suggestions, | ||
coverageScore: coverageScore, | ||
healthSuggestions: | ||
healthSuggestions.isEmpty ? null : healthSuggestions, | ||
maintenanceSuggestions: | ||
maintenanceSuggestions.isEmpty ? null : maintenanceSuggestions, | ||
)); | ||
await scoreCardBackend.updateScoreCard(job.packageName, job.packageVersion); | ||
|
||
|
@@ -381,4 +424,19 @@ class DartdocJobProcessor extends JobProcessor { | |
await tmpTar.rename(p.join(outputDir, _archiveFilePath)); | ||
_appendLog(logFileOutput, pr); | ||
} | ||
|
||
Future<PubDartdocData> _loadPubDartdocData(String outputDir) async { | ||
final file = new File(p.join(outputDir, _pubDataFileName)); | ||
if (!file.existsSync()) { | ||
return null; | ||
} | ||
try { | ||
final content = await file.readAsString(); | ||
return new PubDartdocData.fromJson( | ||
convert.json.decode(content) as Map<String, dynamic>); | ||
} catch (e, st) { | ||
_logger.warning('Unable to parse $_pubDataFileName.', e, st); | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Does $outputDir matter for the error message? There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. It does not, as it will be deleted after the processing is done. The logger's name will contain the package name and version, and we should be able to replicate the issue. |
||
return null; | ||
} | ||
} | ||
} |
Some generated files are not rendered by default. Learn more about how customized files appear on GitHub.
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 suggestion needs to be documented on the help page under scoring.
Up to 10% penalty sounds good to me.
Is doing this linearly between 0.0 and 1.0 coverage a bit too harsh? Does this punish people for class members / getters that might be too simple to document (final Object parent; /// The parent)? If so, we could maybe have no penalty for coverage e.g. under 0.75. I can't tell without data, I really wonder how packages will fare with this new penalty (this is cool stuff).
@mit-mit Is this the logic we want?
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.
10% (1.0) sounds a bit harsh. Can we look at some popular, mature packages and see what's "standard"?
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 10% is coming from here, and it is also the maintenance penalty for not being able to run dartdoc at all.
At this time this only gets stored in the ScoreCardReport, and it is not yet used in the score that we display on the UI (although it should soon be part of it and be exposed on the UI).
<1% coverage is probably a badly documented package, so how about making that as our initial threshold to emit the warning? I'll do another PR to expose the reports as part of the metrics API, and we can check the popular packages, and fine-tune the threshold.
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.
Sorry, I meant 100% (1.0) coverage sound harsh. I think we should find some partial coverage number above which there is zero deduction.
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.
Let's start with the 0.1 coverage as the threshold. That will be max -1 score in health score and -0.3 score in overall score in the worst cases.