Skip to content

Commit 54cf40d

Browse files
srawlinssrawlins
authored
Add a warning when an unknown directive is parsed in a comment. (#2340)
Add a warning when an unknown directive is parsed in a comment. The warning helpfully mentions when the case of a name is wrong.
1 parent d5d479b commit 54cf40d

File tree

4 files changed

+119
-39
lines changed

4 files changed

+119
-39
lines changed

‎lib/src/model/comment_processable.dart

Lines changed: 54 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -55,9 +55,11 @@ mixin CommentProcessable on Documentable, Warnable, Locatable, SourceCodeMixin {
5555
}
5656

5757
String processCommentDirectives(String docs) {
58+
// The vast, vast majority of doc comments have no directives.
5859
if (!docs.contains('{@')) {
5960
return docs;
6061
}
62+
_checkForUnknownDirectives(docs);
6163
docs = _injectExamples(docs);
6264
docs = _injectYouTube(docs);
6365
docs = _injectAnimations(docs);
@@ -78,6 +80,58 @@ mixin CommentProcessable on Documentable, Warnable, Locatable, SourceCodeMixin {
7880
ModelElementRenderer get modelElementRenderer =>
7981
packageGraph.rendererFactory.modelElementRenderer;
8082

83+
static const _allDirectiveNames = [
84+
'animation',
85+
'end-inject-html',
86+
'end-tool',
87+
'endtemplate',
88+
'example',
89+
'macro',
90+
'inject-html',
91+
'template',
92+
'tool',
93+
'youtube',
94+
95+
// Categorization directives, parsed elsewhere:
96+
'api',
97+
'canonicalFor',
98+
'category',
99+
'image',
100+
'samples',
101+
'subCategory',
102+
103+
// Common Dart annotations which may decorate named parameters:
104+
'deprecated',
105+
'required',
106+
];
107+
108+
static final _nameBreak = RegExp('[\\s}]');
109+
110+
// TODO(srawlins): Implement more checks; see
111+
// https://github.com/dart-lang/dartdoc/issues/1814.
112+
void _checkForUnknownDirectives(String docs) {
113+
var index = 0;
114+
while (true) {
115+
var nameStartIndex = docs.indexOf('{@', index);
116+
if (nameStartIndex == -1) return;
117+
var nameEndIndex = docs.indexOf(_nameBreak, nameStartIndex + 2);
118+
if (nameEndIndex == -1) return;
119+
var name = docs.substring(nameStartIndex + 2, nameEndIndex);
120+
if (!_allDirectiveNames.contains(name)) {
121+
if (_allDirectiveNames.contains(name.toLowerCase())) {
122+
warn(PackageWarning.unknownDirective,
123+
message: "'$name' (use lowercase)");
124+
} else {
125+
warn(PackageWarning.unknownDirective, message: "'$name'");
126+
}
127+
}
128+
// TODO(srawlins): Replace all `replaceAllMapped` usage within this file,
129+
// running regex after regex over [docs], with simple calls here. This has
130+
// interactivity / order-of-operations consequences, so take care.
131+
index = nameEndIndex;
132+
}
133+
}
134+
81135
/// Replace {@tool ...&#125{@end-tool} in API comments with the
82136
/// output of an external tool.
83137
///

‎lib/src/model/package_graph.dart

Lines changed: 25 additions & 22 deletions
Original file line numberDiff line numberDiff line change
@@ -341,95 +341,98 @@ class PackageGraph {
341341
// messages and warn for non-public canonicalization
342342
// errors.
343343
warningMessage =
344-
'no canonical library found for ${warnableName}, not linking';
344+
'no canonical library found for $warnableName, not linking';
345345
break;
346346
case PackageWarning.ambiguousReexport:
347347
// Fix these warnings by adding the original library exporting the
348348
// symbol with --include, by using --auto-include-dependencies,
349349
// or by using --exclude to hide one of the libraries involved
350350
warningMessage =
351-
'ambiguous reexport of ${warnableName}, canonicalization candidates: ${message}';
351+
'ambiguous reexport of $warnableName, canonicalization candidates: $message';
352352
break;
353353
case PackageWarning.noDefiningLibraryFound:
354354
warningMessage =
355-
'could not find the defining library for ${warnableName}; the '
355+
'could not find the defining library for $warnableName; the '
356356
'library may be imported or exported with a non-standard URI';
357357
break;
358358
case PackageWarning.noLibraryLevelDocs:
359359
warningMessage =
360360
'${warnable.fullyQualifiedName} has no library level documentation comments';
361361
break;
362362
case PackageWarning.ambiguousDocReference:
363-
warningMessage = 'ambiguous doc reference ${message}';
363+
warningMessage = 'ambiguous doc reference $message';
364364
break;
365365
case PackageWarning.ignoredCanonicalFor:
366366
warningMessage =
367-
"library says it is {@canonicalFor ${message}} but ${message} can't be canonical there";
367+
"library says it is {@canonicalFor $message} but $message can't be canonical there";
368368
break;
369369
case PackageWarning.packageOrderGivesMissingPackageName:
370370
warningMessage =
371-
"--package-order gives invalid package name: '${message}'";
371+
"--package-order gives invalid package name: '$message'";
372372
break;
373373
case PackageWarning.reexportedPrivateApiAcrossPackages:
374374
warningMessage =
375-
'private API of ${message} is reexported by libraries in other packages: ';
375+
'private API of $message is reexported by libraries in other packages: ';
376376
break;
377377
case PackageWarning.notImplemented:
378378
warningMessage = message;
379379
break;
380380
case PackageWarning.unresolvedDocReference:
381-
warningMessage = 'unresolved doc reference [${message}]';
381+
warningMessage = 'unresolved doc reference [$message]';
382382
referredFromPrefix = 'in documentation inherited from';
383383
break;
384+
case PackageWarning.unknownDirective:
385+
warningMessage = 'undefined directive: $message';
386+
break;
384387
case PackageWarning.unknownMacro:
385-
warningMessage = 'undefined macro [${message}]';
388+
warningMessage = 'undefined macro [$message]';
386389
break;
387390
case PackageWarning.unknownHtmlFragment:
388-
warningMessage = 'undefined HTML fragment identifier [${message}]';
391+
warningMessage = 'undefined HTML fragment identifier [$message]';
389392
break;
390393
case PackageWarning.brokenLink:
391-
warningMessage = 'dartdoc generated a broken link to: ${message}';
394+
warningMessage = 'dartdoc generated a broken link to: $message';
392395
warnablePrefix = 'to element';
393396
referredFromPrefix = 'linked to from';
394397
break;
395398
case PackageWarning.orphanedFile:
396-
warningMessage = 'dartdoc generated a file orphan: ${message}';
399+
warningMessage = 'dartdoc generated a file orphan: $message';
397400
break;
398401
case PackageWarning.unknownFile:
399402
warningMessage =
400-
'dartdoc detected an unknown file in the doc tree: ${message}';
403+
'dartdoc detected an unknown file in the doc tree: $message';
401404
break;
402405
case PackageWarning.missingFromSearchIndex:
403406
warningMessage =
404-
'dartdoc generated a file not in the search index: ${message}';
407+
'dartdoc generated a file not in the search index: $message';
405408
break;
406409
case PackageWarning.typeAsHtml:
407410
// The message for this warning can contain many punctuation and other symbols,
408411
// so bracket with a triple quote for defense.
409-
warningMessage = 'generic type handled as HTML: """${message}"""';
412+
warningMessage = 'generic type handled as HTML: """$message"""';
410413
break;
411414
case PackageWarning.invalidParameter:
412-
warningMessage = 'invalid parameter to dartdoc directive: ${message}';
415+
warningMessage = 'invalid parameter to dartdoc directive: $message';
413416
break;
414417
case PackageWarning.toolError:
415-
warningMessage = 'tool execution failed: ${message}';
418+
warningMessage = 'tool execution failed: $message';
416419
break;
417420
case PackageWarning.deprecated:
418-
warningMessage = 'deprecated dartdoc usage: ${message}';
421+
warningMessage = 'deprecated dartdoc usage: $message';
419422
break;
420423
case PackageWarning.unresolvedExport:
421-
warningMessage = 'unresolved export uri: ${message}';
424+
warningMessage = 'unresolved export uri: $message';
422425
break;
423426
case PackageWarning.duplicateFile:
424-
warningMessage = 'failed to write file at: ${message}';
427+
warningMessage = 'failed to write file at: $message';
425428
warnablePrefix = 'for symbol';
426429
referredFromPrefix = 'conflicting with file already generated by';
427430
break;
428431
case PackageWarning.missingConstantConstructor:
429-
warningMessage = 'constant constructor missing: ${message}';
432+
warningMessage = 'constant constructor missing: $message';
430433
break;
431434
case PackageWarning.missingExampleFile:
432-
warningMessage = 'example file not found: ${message}';
435+
warningMessage = 'example file not found: $message';
433436
break;
434437
}
435438

‎lib/src/warnings.dart

Lines changed: 5 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -177,6 +177,10 @@ final Map<PackageWarning, PackageWarningDefinition> packageWarningDefinitions =
177177
'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'),
178178
PackageWarning.brokenLink: PackageWarningDefinition(PackageWarning.brokenLink,
179179
'broken-link', 'Dartdoc generated a link to a non-existent file'),
180+
PackageWarning.unknownDirective: PackageWarningDefinition(
181+
PackageWarning.unknownDirective,
182+
'unknown-directive',
183+
'A comment contains an unknown directive'),
180184
PackageWarning.unknownMacro: PackageWarningDefinition(
181185
PackageWarning.unknownMacro,
182186
'unknown-macro',
@@ -266,6 +270,7 @@ enum PackageWarning {
266270
packageOrderGivesMissingPackageName,
267271
reexportedPrivateApiAcrossPackages,
268272
unresolvedDocReference,
273+
unknownDirective,
269274
unknownMacro,
270275
unknownHtmlFragment,
271276
brokenLink,

‎test/comment_processable_test.dart

Lines changed: 35 additions & 17 deletions
Original file line numberDiff line numberDiff line change
@@ -28,8 +28,8 @@ void main() {
2828

2929
setUp(() async {
3030
resourceProvider = MemoryResourceProvider();
31-
projectRoot =
32-
resourceProvider.getFolder(resourceProvider.convertPath('/project'));
31+
projectRoot = resourceProvider.getFolder(resourceProvider.pathContext
32+
.canonicalize(resourceProvider.convertPath('/project')));
3333
projectRoot.create();
3434
resourceProvider
3535
.getFile(
@@ -100,6 +100,32 @@ Text.
100100
More text.'''));
101101
});
102102

103+
test('warns when an unknown directive is parsed', () async {
104+
processor.element = _FakeElement(source: _FakeSource(fullName: libFooPath));
105+
await processor.processComment('''
106+
/// Text.
107+
///
108+
/// {@marco name}
109+
''');
110+
verify(processor.packageGraph.warnOnElement(
111+
processor, PackageWarning.unknownDirective,
112+
message: "'marco'"))
113+
.called(1);
114+
});
115+
116+
test('warns when a directive with wrong case is parsed', () async {
117+
processor.element = _FakeElement(source: _FakeSource(fullName: libFooPath));
118+
await processor.processComment('''
119+
/// Text.
120+
///
121+
/// {@youTube url}
122+
''');
123+
verify(processor.packageGraph.warnOnElement(
124+
processor, PackageWarning.unknownDirective,
125+
message: "'youTube' (use lowercase)"))
126+
.called(1);
127+
});
128+
103129
test('processes @animation', () async {
104130
processor.element = _FakeElement(source: _FakeSource(fullName: libFooPath));
105131
var doc = await processor.processComment('''
@@ -369,9 +395,7 @@ End text.'''));
369395
});
370396

371397
test('processes @example with file', () async {
372-
var examplePath = resourceProvider.pathContext.canonicalize(
373-
resourceProvider.pathContext.join(projectRoot.path, 'abc.md'));
374-
resourceProvider.getFile(examplePath).writeAsStringSync('''
398+
projectRoot.getChildAssumingFile('abc.md').writeAsStringSync('''
375399
```
376400
Code snippet
377401
```
@@ -398,9 +422,9 @@ End text.'''));
398422
});
399423

400424
test('processes @example with a region', () async {
401-
var examplePath = resourceProvider.pathContext.canonicalize(
402-
resourceProvider.pathContext.join(projectRoot.path, 'abc-r.md'));
403-
resourceProvider.getFile(examplePath).writeAsStringSync('Markdown text.');
425+
projectRoot
426+
.getChildAssumingFile('abc-r.md')
427+
.writeAsStringSync('Markdown text.');
404428
processor.element = _FakeElement(source: _FakeSource(fullName: libFooPath));
405429
var doc = await processor.processComment('''
406430
/// Text.
@@ -417,9 +441,7 @@ Markdown text.'''));
417441

418442
test('adds language to processed @example with an extension and no lang',
419443
() async {
420-
var examplePath = resourceProvider.pathContext.canonicalize(
421-
resourceProvider.pathContext.join(projectRoot.path, 'abc.html.md'));
422-
resourceProvider.getFile(examplePath).writeAsStringSync('''
444+
projectRoot.getChildAssumingFile('abc.html.md').writeAsStringSync('''
423445
```
424446
Code snippet
425447
```
@@ -447,9 +469,7 @@ End text.'''));
447469

448470
test('adds language to processed @example with a lang and an extension',
449471
() async {
450-
var examplePath = resourceProvider.pathContext.canonicalize(
451-
resourceProvider.pathContext.join(projectRoot.path, 'abc.html.md'));
452-
resourceProvider.getFile(examplePath).writeAsStringSync('''
472+
projectRoot.getChildAssumingFile('abc.html.md').writeAsStringSync('''
453473
```
454474
Code snippet
455475
```
@@ -473,9 +493,7 @@ Code snippet
473493

474494
test('adds language to processed @example with a lang and no extension',
475495
() async {
476-
var examplePath = resourceProvider.pathContext.canonicalize(
477-
resourceProvider.pathContext.join(projectRoot.path, 'abc.md'));
478-
resourceProvider.getFile(examplePath).writeAsStringSync('''
496+
projectRoot.getChildAssumingFile('abc.md').writeAsStringSync('''
479497
```
480498
Code snippet
481499
```

0 commit comments

Comments
 (0)