Skip to content
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.

Sign up for GitHub

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

Add a warning when an unknown directive is parsed in a comment. #2340

Merged
merged 3 commits into from
Sep 11, 2020
Merged

Add a warning when an unknown directive is parsed in a comment. #2340

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
3 commits
Select commit Hold shift + click to select a range
210549e
Add a warning when an unknown directive is parsed in a comment.
srawlins Sep 8, 2020
af6e111
little change
srawlins Sep 10, 2020
35434c4
canonicalize
srawlins Sep 10, 2020
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
54 changes: 54 additions & 0 deletions lib/src/model/comment_processable.dart
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -55,9 +55,11 @@ mixin CommentProcessable on Documentable, Warnable, Locatable, SourceCodeMixin {
}

String processCommentDirectives(String docs) {
// The vast, vast majority of doc comments have no directives.
if (!docs.contains('{@')) {
return docs;
}
_checkForUnknownDirectives(docs);
docs = _injectExamples(docs);
docs = _injectYouTube(docs);
docs = _injectAnimations(docs);
Expand All @@ -78,6 +80,58 @@ mixin CommentProcessable on Documentable, Warnable, Locatable, SourceCodeMixin {
ModelElementRenderer get modelElementRenderer =>
packageGraph.rendererFactory.modelElementRenderer;

static const _allDirectiveNames = [
'animation',
'end-inject-html',
'end-tool',
'endtemplate',
'example',
'macro',
'inject-html',
'template',
'tool',
'youtube',

// Categorization directives, parsed elsewhere:
'api',
'canonicalFor',
'category',
'image',
'samples',
'subCategory',

// Common Dart annotations which may decorate named parameters:
'deprecated',
'required',
];

static final _nameBreak = RegExp('[\\s}]');

// TODO(srawlins): Implement more checks; see
// https://github.com/dart-lang/dartdoc/issues/1814.
void _checkForUnknownDirectives(String docs) {
var index = 0;
while (true) {
var nameStartIndex = docs.indexOf('{@', index);
if (nameStartIndex == -1) return;
var nameEndIndex = docs.indexOf(_nameBreak, nameStartIndex + 2);
if (nameEndIndex == -1) return;
var name = docs.substring(nameStartIndex + 2, nameEndIndex);
if (!_allDirectiveNames.contains(name)) {
if (_allDirectiveNames.contains(name.toLowerCase())) {
warn(PackageWarning.unknownDirective,
message: "'$name' (use lowercase)");
} else {
warn(PackageWarning.unknownDirective, message: "'$name'");
}
}
// TODO(srawlins): Replace all `replaceAllMapped` usage within this file,
// running regex after regex over [docs], with simple calls here. This has
// interactivity / order-of-operations consequences, so take care.
index = nameEndIndex;
}
}

/// Replace {@tool ...&#125{@end-tool} in API comments with the
/// output of an external tool.
///
Expand Down
47 changes: 25 additions & 22 deletions lib/src/model/package_graph.dart
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -341,95 +341,98 @@ class PackageGraph {
// messages and warn for non-public canonicalization
// errors.
warningMessage =
'no canonical library found for ${warnableName}, not linking';
'no canonical library found for $warnableName, not linking';
break;
case PackageWarning.ambiguousReexport:
// Fix these warnings by adding the original library exporting the
// symbol with --include, by using --auto-include-dependencies,
// or by using --exclude to hide one of the libraries involved
warningMessage =
'ambiguous reexport of ${warnableName}, canonicalization candidates: ${message}';
'ambiguous reexport of $warnableName, canonicalization candidates: $message';
break;
case PackageWarning.noDefiningLibraryFound:
warningMessage =
'could not find the defining library for ${warnableName}; the '
'could not find the defining library for $warnableName; the '
'library may be imported or exported with a non-standard URI';
break;
case PackageWarning.noLibraryLevelDocs:
warningMessage =
'${warnable.fullyQualifiedName} has no library level documentation comments';
break;
case PackageWarning.ambiguousDocReference:
warningMessage = 'ambiguous doc reference ${message}';
warningMessage = 'ambiguous doc reference $message';
break;
case PackageWarning.ignoredCanonicalFor:
warningMessage =
"library says it is {@canonicalFor ${message}} but ${message} can't be canonical there";
"library says it is {@canonicalFor $message} but $message can't be canonical there";
break;
case PackageWarning.packageOrderGivesMissingPackageName:
warningMessage =
"--package-order gives invalid package name: '${message}'";
"--package-order gives invalid package name: '$message'";
break;
case PackageWarning.reexportedPrivateApiAcrossPackages:
warningMessage =
'private API of ${message} is reexported by libraries in other packages: ';
'private API of $message is reexported by libraries in other packages: ';
break;
case PackageWarning.notImplemented:
warningMessage = message;
break;
case PackageWarning.unresolvedDocReference:
warningMessage = 'unresolved doc reference [${message}]';
warningMessage = 'unresolved doc reference [$message]';
referredFromPrefix = 'in documentation inherited from';
break;
case PackageWarning.unknownDirective:
warningMessage = 'undefined directive: $message';
break;
case PackageWarning.unknownMacro:
warningMessage = 'undefined macro [${message}]';
warningMessage = 'undefined macro [$message]';
break;
case PackageWarning.unknownHtmlFragment:
warningMessage = 'undefined HTML fragment identifier [${message}]';
warningMessage = 'undefined HTML fragment identifier [$message]';
break;
case PackageWarning.brokenLink:
warningMessage = 'dartdoc generated a broken link to: ${message}';
warningMessage = 'dartdoc generated a broken link to: $message';
warnablePrefix = 'to element';
referredFromPrefix = 'linked to from';
break;
case PackageWarning.orphanedFile:
warningMessage = 'dartdoc generated a file orphan: ${message}';
warningMessage = 'dartdoc generated a file orphan: $message';
break;
case PackageWarning.unknownFile:
warningMessage =
'dartdoc detected an unknown file in the doc tree: ${message}';
'dartdoc detected an unknown file in the doc tree: $message';
break;
case PackageWarning.missingFromSearchIndex:
warningMessage =
'dartdoc generated a file not in the search index: ${message}';
'dartdoc generated a file not in the search index: $message';
break;
case PackageWarning.typeAsHtml:
// The message for this warning can contain many punctuation and other symbols,
// so bracket with a triple quote for defense.
warningMessage = 'generic type handled as HTML: """${message}"""';
warningMessage = 'generic type handled as HTML: """$message"""';
break;
case PackageWarning.invalidParameter:
warningMessage = 'invalid parameter to dartdoc directive: ${message}';
warningMessage = 'invalid parameter to dartdoc directive: $message';
break;
case PackageWarning.toolError:
warningMessage = 'tool execution failed: ${message}';
warningMessage = 'tool execution failed: $message';
break;
case PackageWarning.deprecated:
warningMessage = 'deprecated dartdoc usage: ${message}';
warningMessage = 'deprecated dartdoc usage: $message';
break;
case PackageWarning.unresolvedExport:
warningMessage = 'unresolved export uri: ${message}';
warningMessage = 'unresolved export uri: $message';
break;
case PackageWarning.duplicateFile:
warningMessage = 'failed to write file at: ${message}';
warningMessage = 'failed to write file at: $message';
warnablePrefix = 'for symbol';
referredFromPrefix = 'conflicting with file already generated by';
break;
case PackageWarning.missingConstantConstructor:
warningMessage = 'constant constructor missing: ${message}';
warningMessage = 'constant constructor missing: $message';
break;
case PackageWarning.missingExampleFile:
warningMessage = 'example file not found: ${message}';
warningMessage = 'example file not found: $message';
break;
}

Expand Down
5 changes: 5 additions & 0 deletions lib/src/warnings.dart
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -177,6 +177,10 @@ final Map<PackageWarning, PackageWarningDefinition> packageWarningDefinitions =
'A comment reference could not be found in parameters, enclosing class, enclosing library, or at the top level of any documented library with the package'),
PackageWarning.brokenLink: PackageWarningDefinition(PackageWarning.brokenLink,
'broken-link', 'Dartdoc generated a link to a non-existent file'),
PackageWarning.unknownDirective: PackageWarningDefinition(
PackageWarning.unknownDirective,
'unknown-directive',
'A comment contains an unknown directive'),
PackageWarning.unknownMacro: PackageWarningDefinition(
PackageWarning.unknownMacro,
'unknown-macro',
Expand Down Expand Up @@ -266,6 +270,7 @@ enum PackageWarning {
packageOrderGivesMissingPackageName,
reexportedPrivateApiAcrossPackages,
unresolvedDocReference,
unknownDirective,
unknownMacro,
unknownHtmlFragment,
brokenLink,
Expand Down
52 changes: 35 additions & 17 deletions test/comment_processable_test.dart
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -28,8 +28,8 @@ void main() {

setUp(() async {
resourceProvider = MemoryResourceProvider();
projectRoot =
resourceProvider.getFolder(resourceProvider.convertPath('/project'));
projectRoot = resourceProvider.getFolder(resourceProvider.pathContext
.canonicalize(resourceProvider.convertPath('/project')));
projectRoot.create();
resourceProvider
.getFile(
Expand Down Expand Up @@ -100,6 +100,32 @@ Text.
More text.'''));
});

test('warns when an unknown directive is parsed', () async {
processor.element = _FakeElement(source: _FakeSource(fullName: libFooPath));
await processor.processComment('''
/// Text.
///
/// {@marco name}
''');
verify(processor.packageGraph.warnOnElement(
processor, PackageWarning.unknownDirective,
message: "'marco'"))
.called(1);
});

test('warns when a directive with wrong case is parsed', () async {
processor.element = _FakeElement(source: _FakeSource(fullName: libFooPath));
await processor.processComment('''
/// Text.
///
/// {@youTube url}
''');
verify(processor.packageGraph.warnOnElement(
processor, PackageWarning.unknownDirective,
message: "'youTube' (use lowercase)"))
.called(1);
});

test('processes @animation', () async {
processor.element = _FakeElement(source: _FakeSource(fullName: libFooPath));
var doc = await processor.processComment('''
Expand Down Expand Up @@ -369,9 +395,7 @@ End text.'''));
});

test('processes @example with file', () async {
var examplePath = resourceProvider.pathContext.canonicalize(
resourceProvider.pathContext.join(projectRoot.path, 'abc.md'));
resourceProvider.getFile(examplePath).writeAsStringSync('''
projectRoot.getChildAssumingFile('abc.md').writeAsStringSync('''
```
Code snippet
```
Expand All @@ -398,9 +422,9 @@ End text.'''));
});

test('processes @example with a region', () async {
var examplePath = resourceProvider.pathContext.canonicalize(
resourceProvider.pathContext.join(projectRoot.path, 'abc-r.md'));
resourceProvider.getFile(examplePath).writeAsStringSync('Markdown text.');
projectRoot
.getChildAssumingFile('abc-r.md')
.writeAsStringSync('Markdown text.');
processor.element = _FakeElement(source: _FakeSource(fullName: libFooPath));
var doc = await processor.processComment('''
/// Text.
Expand All @@ -417,9 +441,7 @@ Markdown text.'''));

test('adds language to processed @example with an extension and no lang',
() async {
var examplePath = resourceProvider.pathContext.canonicalize(
resourceProvider.pathContext.join(projectRoot.path, 'abc.html.md'));
resourceProvider.getFile(examplePath).writeAsStringSync('''
projectRoot.getChildAssumingFile('abc.html.md').writeAsStringSync('''
```
Code snippet
```
Expand Down Expand Up @@ -447,9 +469,7 @@ End text.'''));

test('adds language to processed @example with a lang and an extension',
() async {
var examplePath = resourceProvider.pathContext.canonicalize(
resourceProvider.pathContext.join(projectRoot.path, 'abc.html.md'));
resourceProvider.getFile(examplePath).writeAsStringSync('''
projectRoot.getChildAssumingFile('abc.html.md').writeAsStringSync('''
```
Code snippet
```
Expand All @@ -473,9 +493,7 @@ Code snippet

test('adds language to processed @example with a lang and no extension',
() async {
var examplePath = resourceProvider.pathContext.canonicalize(
resourceProvider.pathContext.join(projectRoot.path, 'abc.md'));
resourceProvider.getFile(examplePath).writeAsStringSync('''
projectRoot.getChildAssumingFile('abc.md').writeAsStringSync('''
```
Code snippet
```
Expand Down