-
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
Conversation
isoos
commented
Oct 17, 2018
- Dartdoc coverage is part of health score, but broken dartdoc execution is part of maintenance score. They need to be tracked and calculated separately.
- Coverage calculation is simple: documented/total (the number of API elements).
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
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 comment
The 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 comment
The 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.
'$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), |
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.
88d4551
to
6132bae
Compare
6132bae
to
761fa90
Compare