Skip to content

Commit dc502d0

Browse files
srawlinssrawlins
authored
Move much PackageWarning logic _out_ of PackageGraph, into the enum. (#3251)
1 parent ad651b1 commit dc502d0

File tree

3 files changed

+113
-154
lines changed

3 files changed

+113
-154
lines changed

‎lib/src/model/locatable.dart

Lines changed: 5 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -22,3 +22,8 @@ abstract class Locatable {
2222
}
2323

2424
final RegExp locationSplitter = RegExp(r'(package:|[\\/;.])');
25+
26+
extension NullableLocatable on Locatable? {
27+
String get safeWarnableName =>
28+
this?.fullyQualifiedName.replaceFirst(':', '-') ?? '<unknown>';
29+
}

‎lib/src/model/package_graph.dart

Lines changed: 32 additions & 127 deletions
Original file line numberDiff line numberDiff line change
@@ -292,8 +292,8 @@ class PackageGraph with CommentReferable, Nameable, ModelBuilder {
292292

293293
void warnOnElement(Warnable? warnable, PackageWarning kind,
294294
{String? message,
295-
Iterable<Locatable>? referredFrom,
296-
Iterable<String>? extendedDebug}) {
295+
Iterable<Locatable> referredFrom = const [],
296+
Iterable<String> extendedDebug = const []}) {
297297
var newEntry = Tuple3(warnable?.element, kind, message);
298298
if (_warnAlreadySeen.contains(newEntry)) {
299299
return;
@@ -310,16 +310,14 @@ class PackageGraph with CommentReferable, Nameable, ModelBuilder {
310310

311311
void _warnOnElement(Warnable? warnable, PackageWarning kind,
312312
{required String message,
313-
Iterable<Locatable>? referredFrom,
314-
Iterable<String>? extendedDebug}) {
315-
if (warnable is ModelElement) {
313+
Iterable<Locatable> referredFrom = const [],
314+
Iterable<String> extendedDebug = const []}) {
315+
if (warnable is ModelElement && kind == PackageWarning.ambiguousReexport) {
316316
// This sort of warning is only applicable to top level elements.
317-
if (kind == PackageWarning.ambiguousReexport) {
318-
var enclosingElement = warnable.enclosingElement;
319-
while (enclosingElement != null && enclosingElement is! Library) {
320-
warnable = enclosingElement;
321-
enclosingElement = warnable.enclosingElement;
322-
}
317+
var enclosingElement = warnable.enclosingElement;
318+
while (enclosingElement != null && enclosingElement is! Library) {
319+
warnable = enclosingElement;
320+
enclosingElement = warnable.enclosingElement;
323321
}
324322
}
325323

@@ -329,165 +327,72 @@ class PackageGraph with CommentReferable, Nameable, ModelBuilder {
329327
if (packageWarningCounter.hasWarning(warnable, kind, message)) {
330328
return;
331329
}
332-
// Some kinds of warnings it is OK to drop if we're not documenting them.
330+
// Some kinds of warnings are OK to drop if we're not documenting them.
333331
// TODO(jcollins-g): drop this and use new flag system instead.
334332
if (warnable != null &&
335333
skipWarningIfNotDocumentedFor.contains(kind) &&
336334
!warnable.isDocumented) {
337335
return;
338336
}
339337
// Elements that are part of the Dart SDK can have colons in their FQNs.
340-
// This confuses IntelliJ and makes it so it can't link to the location
341-
// of the error in the console window, so separate out the library from
342-
// the path.
343-
// TODO(jcollins-g): What about messages that may include colons? Substituting
344-
// them out doesn't work as well there since it might confuse
345-
// the user, yet we still want IntelliJ to link properly.
346-
final warnableName = _safeWarnableName(warnable);
347-
348-
var warnablePrefix = 'from';
349-
var referredFromPrefix = 'referred to by';
338+
// This confuses IntelliJ and makes it so it can't link to the location of
339+
// the error in the console window, so separate out the library from the
340+
// path.
341+
// TODO(jcollins-g): What about messages that may include colons?
342+
// Substituting them out doesn't work as well there since it might confuse
343+
// the user, yet we still want IntelliJ to link properly.
344+
final warnableName = warnable.safeWarnableName;
345+
350346
String warningMessage;
351347
switch (kind) {
352-
case PackageWarning.noCanonicalFound:
353-
// Fix these warnings by adding libraries with --include, or by using
354-
// --auto-include-dependencies.
355-
// TODO(jcollins-g): pipeline references through linkedName for error
356-
// messages and warn for non-public canonicalization
357-
// errors.
358-
warningMessage =
359-
'no canonical library found for $warnableName, not linking';
360-
break;
361348
case PackageWarning.ambiguousReexport:
362-
// Fix these warnings by adding the original library exporting the
363-
// symbol with --include, by using --auto-include-dependencies,
364-
// or by using --exclude to hide one of the libraries involved
365-
warningMessage =
366-
'ambiguous reexport of $warnableName, canonicalization candidates: $message';
349+
warningMessage = kind.messageFor([warnableName, message]);
367350
break;
351+
case PackageWarning.noCanonicalFound:
368352
case PackageWarning.noDefiningLibraryFound:
369-
warningMessage =
370-
'could not find the defining library for $warnableName; the '
371-
'library may be imported or exported with a non-standard URI';
353+
warningMessage = kind.messageFor([warnableName]);
372354
break;
373355
case PackageWarning.noLibraryLevelDocs:
374-
warningMessage =
375-
'${warnable!.fullyQualifiedName} has no library level documentation comments';
376-
break;
377356
case PackageWarning.noDocumentableLibrariesInPackage:
378-
warningMessage =
379-
'${warnable!.fullyQualifiedName} has no documentable libraries';
357+
warningMessage = kind.messageFor([warnable!.fullyQualifiedName]);
380358
break;
381359
case PackageWarning.ambiguousDocReference:
382-
warningMessage = 'ambiguous doc reference $message';
383-
break;
384360
case PackageWarning.ignoredCanonicalFor:
385-
warningMessage =
386-
"library says it is {@canonicalFor $message} but $message can't be canonical there";
387-
break;
388361
case PackageWarning.packageOrderGivesMissingPackageName:
389-
warningMessage =
390-
"--package-order gives invalid package name: '$message'";
391-
break;
392362
case PackageWarning.reexportedPrivateApiAcrossPackages:
393-
warningMessage =
394-
'private API of $message is reexported by libraries in other packages: ';
395-
break;
396363
case PackageWarning.notImplemented:
397-
warningMessage = message;
398-
break;
399364
case PackageWarning.unresolvedDocReference:
400-
warningMessage = 'unresolved doc reference [$message]';
401-
referredFromPrefix = 'in documentation inherited from';
402-
break;
403365
case PackageWarning.unknownDirective:
404-
warningMessage = 'undefined directive: $message';
405-
break;
406366
case PackageWarning.unknownMacro:
407-
warningMessage = 'undefined macro [$message]';
408-
break;
409367
case PackageWarning.unknownHtmlFragment:
410-
warningMessage = 'undefined HTML fragment identifier [$message]';
411-
break;
412368
case PackageWarning.brokenLink:
413-
warningMessage = 'dartdoc generated a broken link to: $message';
414-
warnablePrefix = 'to element';
415-
referredFromPrefix = 'linked to from';
416-
break;
369+
case PackageWarning.duplicateFile:
417370
case PackageWarning.orphanedFile:
418-
warningMessage = 'dartdoc generated a file orphan: $message';
419-
break;
420371
case PackageWarning.unknownFile:
421-
warningMessage =
422-
'dartdoc detected an unknown file in the doc tree: $message';
423-
break;
424372
case PackageWarning.missingFromSearchIndex:
425-
warningMessage =
426-
'dartdoc generated a file not in the search index: $message';
427-
break;
428373
case PackageWarning.typeAsHtml:
429-
// The message for this warning can contain many punctuation and other symbols,
430-
// so bracket with a triple quote for defense.
431-
warningMessage = 'generic type handled as HTML: """$message"""';
432-
break;
433374
case PackageWarning.invalidParameter:
434-
warningMessage = 'invalid parameter to dartdoc directive: $message';
435-
break;
436375
case PackageWarning.toolError:
437-
warningMessage = 'tool execution failed: $message';
438-
break;
439376
case PackageWarning.deprecated:
440-
warningMessage = 'deprecated dartdoc usage: $message';
441-
break;
442377
case PackageWarning.unresolvedExport:
443-
warningMessage = 'unresolved export uri: $message';
444-
break;
445-
case PackageWarning.duplicateFile:
446-
warningMessage = 'failed to write file at: $message';
447-
warnablePrefix = 'for symbol';
448-
referredFromPrefix = 'conflicting with file already generated by';
449-
break;
450378
case PackageWarning.missingConstantConstructor:
451-
warningMessage = 'constant constructor missing: $message';
452-
break;
453379
case PackageWarning.missingExampleFile:
454-
warningMessage = 'example file not found: $message';
455-
break;
456380
case PackageWarning.missingCodeBlockLanguage:
457-
warningMessage = 'missing code block language: $message';
458-
break;
381+
warningMessage = kind.messageFor([message]);
459382
}
460383

461-
var messageParts = <String>[warningMessage];
462-
if (warnable != null) {
463-
messageParts.add('$warnablePrefix $warnableName: ${warnable.location}');
464-
}
465-
if (referredFrom != null) {
466-
for (var referral in referredFrom) {
467-
if (referral != warnable) {
468-
var referredFromStrings = _safeWarnableName(referral);
469-
messageParts.add(
470-
'$referredFromPrefix $referredFromStrings: ${referral.location}');
471-
}
472-
}
473-
}
474-
if (config.verboseWarnings && extendedDebug != null) {
475-
messageParts.addAll(extendedDebug.map((s) => ' $s'));
476-
}
477-
var fullMessage = messageParts.join('\n ');
384+
var fullMessage = [
385+
warningMessage,
386+
if (warnable != null) kind.messageForWarnable(warnable),
387+
for (var referral in referredFrom)
388+
if (referral != warnable) kind.messageForReferral(referral),
389+
if (config.verboseWarnings) ...extendedDebug.map((s) => ' $s')
390+
].join('\n ');
478391

479392
packageWarningCounter.addWarning(warnable, kind, message, fullMessage);
480393
}
481394

482-
String _safeWarnableName(Locatable? locatable) {
483-
if (locatable == null) {
484-
return '<unknown>';
485-
}
486-
487-
return locatable.fullyQualifiedName.replaceFirst(':', '-');
488-
}
489-
490-
List<Package> get packages => packageMap.values.toList(growable: false);
395+
Iterable<Package> get packages => packageMap.values;
491396

492397
late final List<Package> publicPackages = () {
493398
assert(allLibrariesAdded);

‎lib/src/warnings.dart

Lines changed: 76 additions & 27 deletions
Original file line numberDiff line numberDiff line change
@@ -313,33 +313,82 @@ mixin Warnable implements Canonicalization, CommentReferable {
313313

314314
// The kinds of warnings that can be displayed when documenting a package.
315315
enum PackageWarning {
316-
ambiguousDocReference,
317-
ambiguousReexport,
318-
ignoredCanonicalFor,
319-
noCanonicalFound,
320-
noDefiningLibraryFound,
321-
notImplemented,
322-
noDocumentableLibrariesInPackage,
323-
noLibraryLevelDocs,
324-
packageOrderGivesMissingPackageName,
325-
reexportedPrivateApiAcrossPackages,
326-
unresolvedDocReference,
327-
unknownDirective,
328-
unknownMacro,
329-
unknownHtmlFragment,
330-
brokenLink,
331-
duplicateFile,
332-
orphanedFile,
333-
unknownFile,
334-
missingFromSearchIndex,
335-
typeAsHtml,
336-
invalidParameter,
337-
toolError,
338-
deprecated,
339-
unresolvedExport,
340-
missingConstantConstructor,
341-
missingExampleFile,
342-
missingCodeBlockLanguage,
316+
ambiguousDocReference('ambiguous doc reference {0}'),
317+
318+
// Fix these warnings by adding the original library exporting the symbol with
319+
// `--include`, by using `--auto-include-dependencies`, or by using
320+
// `--exclude` to hide one of the libraries involved.
321+
ambiguousReexport(
322+
'ambiguous reexport of {0}, canonicalization candidates: {1}'),
323+
ignoredCanonicalFor(
324+
"library says it is {@canonicalFor {0}} but {0} can't be canonical "
325+
'there'),
326+
327+
// Fix these warnings by adding libraries with `--include`, or by using
328+
// `--auto-include-dependencies`.
329+
// TODO(jcollins-g): pipeline references through `linkedName` for error
330+
// messages and warn for non-public canonicalization errors.
331+
noCanonicalFound('no canonical library found for {0}, not linking'),
332+
noDefiningLibraryFound('could not find the defining library for {0}; the '
333+
'library may be imported or exported with a non-standard URI'),
334+
notImplemented('{0}'),
335+
noDocumentableLibrariesInPackage('{0} has no documentable libraries'),
336+
noLibraryLevelDocs('{0} has no library level documentation comments'),
337+
packageOrderGivesMissingPackageName(
338+
"--package-order gives invalid package name: '{0}'"),
339+
reexportedPrivateApiAcrossPackages(
340+
'private API of {0} is reexported by libraries in other packages: '),
341+
unresolvedDocReference('unresolved doc reference [{0}]',
342+
referredFromPrefix: 'in documentation inherited from'),
343+
unknownDirective('undefined directive: {0}'),
344+
unknownMacro('undefined macro [{0}]'),
345+
unknownHtmlFragment('undefined HTML fragment identifier [{0}]'),
346+
brokenLink('dartdoc generated a broken link to: {0}',
347+
warnablePrefix: 'to element', referredFromPrefix: 'linked to from'),
348+
duplicateFile('failed to write file at: {0}',
349+
warnablePrefix: 'for symbol',
350+
referredFromPrefix: 'conflicting with file already generated by'),
351+
orphanedFile('dartdoc generated a file orphan: {0}'),
352+
unknownFile('dartdoc detected an unknown file in the doc tree: {0}'),
353+
missingFromSearchIndex(
354+
'dartdoc generated a file not in the search index: {0}'),
355+
356+
// The message for this warning can contain many punctuation and other
357+
// symbols, so bracket with a triple quote for defense.
358+
typeAsHtml('generic type handled as HTML: """{0}"""'),
359+
invalidParameter('invalid parameter to dartdoc directive: {0}'),
360+
toolError('tool execution failed: {0}'),
361+
deprecated('deprecated dartdoc usage: {0}'),
362+
unresolvedExport('unresolved export uri: {0}'),
363+
missingConstantConstructor('constant constructor missing: {0}'),
364+
missingExampleFile('example file not found: {0}'),
365+
missingCodeBlockLanguage('missing code block language: {0}');
366+
367+
final String template;
368+
369+
final String warnablePrefix;
370+
371+
final String referredFromPrefix;
372+
373+
const PackageWarning(
374+
this.template, {
375+
this.warnablePrefix = 'from',
376+
this.referredFromPrefix = 'referred to by',
377+
});
378+
379+
String messageFor(List<String> messageParts) {
380+
var message = template;
381+
for (var i = 0; i < messageParts.length; i++) {
382+
message = message.replaceAll('{$i}', messageParts[i]);
383+
}
384+
return message;
385+
}
386+
387+
String messageForWarnable(Warnable warnable) =>
388+
'$warnablePrefix ${warnable.safeWarnableName}: ${warnable.location}';
389+
390+
String messageForReferral(Locatable referral) =>
391+
'$referredFromPrefix ${referral.safeWarnableName}: ${referral.location}';
343392
}
344393

345394
/// Used to declare defaults for a particular package warning.

0 commit comments

Comments
 (0)