From ab08e71746a2fcd81c4a00b22713cb60257a12af Mon Sep 17 00:00:00 2001 From: Janice Collins Date: Fri, 3 Jan 2020 15:56:38 -0800 Subject: [PATCH 1/2] Do not assert on multiple file crashes, use PackageWarning error instead. --- lib/dartdoc.dart | 2 +- lib/src/empty_generator.dart | 5 +-- lib/src/generator.dart | 3 +- lib/src/html/html_generator.dart | 22 ++++++++---- lib/src/html/html_generator_instance.dart | 6 ++-- lib/src/model/package_graph.dart | 5 +++ lib/src/warnings.dart | 14 ++++++++ test/html_generator_test.dart | 35 +++++++++++++++++++ test/src/utils.dart | 2 ++ testing/test_package_duplicate/lib/first.dart | 1 + .../test_package_duplicate/lib/second.dart | 1 + testing/test_package_duplicate/pubspec.yaml | 1 + 12 files changed, 84 insertions(+), 13 deletions(-) create mode 100644 testing/test_package_duplicate/lib/first.dart create mode 100644 testing/test_package_duplicate/lib/second.dart create mode 100644 testing/test_package_duplicate/pubspec.yaml diff --git a/lib/dartdoc.dart b/lib/dartdoc.dart index 0a8e252668..ee99d1b0c3 100644 --- a/lib/dartdoc.dart +++ b/lib/dartdoc.dart @@ -105,7 +105,7 @@ class Dartdoc extends PackageBuilder { for (var generator in generators) { await generator.generate(packageGraph, outputDir.path); - writtenFiles.addAll(generator.writtenFiles.map(path.normalize)); + writtenFiles.addAll(generator.writtenFiles.keys.map(path.normalize)); } if (config.validateLinks && writtenFiles.isNotEmpty) { validateLinks(packageGraph, outputDir.path); diff --git a/lib/src/empty_generator.dart b/lib/src/empty_generator.dart index 016f2d1062..d87957400c 100644 --- a/lib/src/empty_generator.dart +++ b/lib/src/empty_generator.dart @@ -5,9 +5,10 @@ import 'dart:async'; import 'package:dartdoc/src/generator.dart'; import 'package:dartdoc/src/model/model.dart'; import 'package:dartdoc/src/model_utils.dart'; +import 'package:dartdoc/src/warnings.dart'; /// A generator that does not generate files, but does traverse the [PackageGraph] -/// and access [ModelElement.documetationAsHtml] for every element as though +/// and access [ModelElement.documentationAsHtml] for every element as though /// it were. class EmptyGenerator extends Generator { @override @@ -36,5 +37,5 @@ class EmptyGenerator extends Generator { Stream get onFileCreated => _onFileCreated.stream; @override - Set get writtenFiles => Set(); + final Map writtenFiles = {}; } diff --git a/lib/src/generator.dart b/lib/src/generator.dart index 758659a022..d062ae97c5 100644 --- a/lib/src/generator.dart +++ b/lib/src/generator.dart @@ -8,6 +8,7 @@ library dartdoc.generator; import 'dart:async' show Stream, Future; import 'package:dartdoc/src/model/model.dart' show PackageGraph; +import 'package:dartdoc/src/warnings.dart'; /// An abstract class that defines a generator that generates documentation for /// a given package. @@ -22,5 +23,5 @@ abstract class Generator { Stream get onFileCreated; /// Fetches all filenames written by this generator. - Set get writtenFiles; + Map get writtenFiles; } diff --git a/lib/src/html/html_generator.dart b/lib/src/html/html_generator.dart index f346499cf7..ca9b0c6f0a 100644 --- a/lib/src/html/html_generator.dart +++ b/lib/src/html/html_generator.dart @@ -15,6 +15,7 @@ import 'package:dartdoc/src/html/html_generator_instance.dart'; import 'package:dartdoc/src/html/template_data.dart'; import 'package:dartdoc/src/html/templates.dart'; import 'package:dartdoc/src/model/model.dart'; +import 'package:dartdoc/src/warnings.dart'; import 'package:path/path.dart' as path; typedef Renderer = String Function(String input); @@ -48,7 +49,7 @@ class HtmlGenerator extends Generator { Stream get onFileCreated => _onFileCreated.stream; @override - final Set writtenFiles = Set(); + final Map writtenFiles = {}; static Future create( {HtmlGeneratorOptions options, @@ -83,14 +84,23 @@ class HtmlGenerator extends Generator { assert(_instance == null); var enabled = true; - void write(String filePath, Object content, {bool allowOverwrite}) { + void write(String filePath, Object content, + {bool allowOverwrite, Warnable element}) { allowOverwrite ??= false; if (!enabled) { throw StateError('`write` was called after `generate` completed.'); } - // If you see this assert, we're probably being called to build non-canonical - // docs somehow. Check data.self.isCanonical and callers for bugs. - assert(allowOverwrite || !writtenFiles.contains(filePath)); + if (!allowOverwrite) { + if (writtenFiles.containsKey(filePath)) { + assert(element != null, + 'Attempted overwrite of ${filePath} without corresponding element'); + Warnable originalElement = writtenFiles[filePath]; + Iterable referredFrom = + originalElement != null ? [originalElement] : null; + element?.warn(PackageWarning.duplicateFile, + message: filePath, referredFrom: referredFrom); + } + } var file = File(path.join(outputDirectoryPath, filePath)); var parent = file.parent; @@ -107,7 +117,7 @@ class HtmlGenerator extends Generator { content, 'content', '`content` must be `String` or `List`.'); } _onFileCreated.add(file); - writtenFiles.add(filePath); + writtenFiles[filePath] = element; } try { diff --git a/lib/src/html/html_generator_instance.dart b/lib/src/html/html_generator_instance.dart index b3adb6cc07..0ac5d7fda5 100644 --- a/lib/src/html/html_generator_instance.dart +++ b/lib/src/html/html_generator_instance.dart @@ -21,7 +21,7 @@ import 'package:mustache/mustache.dart'; import 'package:path/path.dart' as path; typedef FileWriter = void Function(String path, Object content, - {bool allowOverwrite}); + {bool allowOverwrite, Warnable element}); class HtmlGeneratorInstance { final HtmlGeneratorOptions _options; @@ -412,8 +412,8 @@ class HtmlGeneratorInstance { // Replaces '/' separators with proper separators for the platform. String outFile = path.joinAll(filename.split('/')); String content = template.renderString(data); - - _writer(outFile, content); + _writer(outFile, content, + element: data.self is Warnable ? data.self : null); if (data.self is Indexable) _indexedElements.add(data.self as Indexable); } } diff --git a/lib/src/model/package_graph.dart b/lib/src/model/package_graph.dart index 1025e27861..83043bb709 100644 --- a/lib/src/model/package_graph.dart +++ b/lib/src/model/package_graph.dart @@ -405,6 +405,11 @@ class PackageGraph { case PackageWarning.unresolvedExport: warningMessage = 'unresolved export uri: ${message}'; break; + case PackageWarning.duplicateFile: + warningMessage = 'failed to write file at: ${message}'; + warnablePrefix = 'for symbol'; + referredFromPrefix = 'conflicting with file already generated by'; + break; } List messageParts = [warningMessage]; diff --git a/lib/src/warnings.dart b/lib/src/warnings.dart index 2d59d7be22..e1941fbdf0 100644 --- a/lib/src/warnings.dart +++ b/lib/src/warnings.dart @@ -193,6 +193,19 @@ final Map packageWarningDefinitions = "unresolved-export", "An export refers to a URI that cannot be resolved.", defaultWarningMode: PackageWarningMode.error), + PackageWarning.duplicateFile: PackageWarningDefinition( + PackageWarning.duplicateFile, + "duplicate-file", + "Dartdoc is trying to write to a duplicate filename based on the names of Dart symbols.", + longHelp: [ + "Dartdoc generates a path and filename to write to for each symbol.", + "@@name@@ conflicts with another symbol in the generated path, and", + "therefore can not be written out. Changing the name, library name, or", + "class name (if appropriate) of one of the conflicting items can resolve", + "the conflict. Alternatively, use the @nodoc tag in one symbol's", + "documentation comments to hide it." + ], + defaultWarningMode: PackageWarningMode.error), }; /// Something that package warnings can be called on. Optionally associated @@ -222,6 +235,7 @@ enum PackageWarning { unknownMacro, unknownHtmlFragment, brokenLink, + duplicateFile, orphanedFile, unknownFile, missingFromSearchIndex, diff --git a/test/html_generator_test.dart b/test/html_generator_test.dart index cda62d7a12..6cfdee0dd8 100644 --- a/test/html_generator_test.dart +++ b/test/html_generator_test.dart @@ -9,9 +9,13 @@ import 'dart:io' show File, Directory; import 'package:dartdoc/src/html/html_generator.dart'; import 'package:dartdoc/src/html/templates.dart'; import 'package:dartdoc/src/html/resources.g.dart'; +import 'package:dartdoc/src/model/package_graph.dart'; +import 'package:dartdoc/src/warnings.dart'; import 'package:path/path.dart' as path; import 'package:test/test.dart'; +import 'src/utils.dart' as utils; + void main() { group('Templates', () { Templates templates; @@ -90,6 +94,37 @@ void main() { } }); }); + + group('for a package that causes duplicate files', () { + HtmlGenerator generator; + PackageGraph packageGraph; + Directory tempOutput; + + setUp(() async { + generator = await HtmlGenerator.create(); + packageGraph = await utils + .bootBasicPackage(utils.testPackageDuplicateDir.path, []); + tempOutput = await Directory.systemTemp.createTemp('doc_test_temp'); + }); + + tearDown(() { + if (tempOutput != null) { + tempOutput.deleteSync(recursive: true); + } + }); + + test('run generator and verify duplicate file error', () async { + await generator.generate(packageGraph, tempOutput.path); + expect(generator, isNotNull); + expect(tempOutput, isNotNull); + String expectedPath = + path.join('aDuplicate', 'aDuplicate-library.html'); + expect( + packageGraph.localPublicLibraries, + anyElement((l) => packageGraph.packageWarningCounter + .hasWarning(l, PackageWarning.duplicateFile, expectedPath))); + }, timeout: Timeout.factor(2)); + }); }); } diff --git a/test/src/utils.dart b/test/src/utils.dart index f387dcc46f..b5d8a793df 100644 --- a/test/src/utils.dart +++ b/test/src/utils.dart @@ -59,6 +59,8 @@ Future get testPackageGraphSdk => final Directory testPackageBadDir = Directory('testing/test_package_bad'); final Directory testPackageDir = Directory('testing/test_package'); +final Directory testPackageDuplicateDir = + Directory('testing/test_package_duplicate'); final Directory testPackageExperimentsDir = Directory('testing/test_package_experiments'); final Directory testPackageMinimumDir = diff --git a/testing/test_package_duplicate/lib/first.dart b/testing/test_package_duplicate/lib/first.dart new file mode 100644 index 0000000000..d369bdaa3c --- /dev/null +++ b/testing/test_package_duplicate/lib/first.dart @@ -0,0 +1 @@ +library aDuplicate; diff --git a/testing/test_package_duplicate/lib/second.dart b/testing/test_package_duplicate/lib/second.dart new file mode 100644 index 0000000000..d369bdaa3c --- /dev/null +++ b/testing/test_package_duplicate/lib/second.dart @@ -0,0 +1 @@ +library aDuplicate; diff --git a/testing/test_package_duplicate/pubspec.yaml b/testing/test_package_duplicate/pubspec.yaml new file mode 100644 index 0000000000..f0f27294b7 --- /dev/null +++ b/testing/test_package_duplicate/pubspec.yaml @@ -0,0 +1 @@ +name: test_package_duplicate From 09a4e8b3678458a1e67efcb37a32fdaa1c92b7fd Mon Sep 17 00:00:00 2001 From: Janice Collins Date: Fri, 3 Jan 2020 16:01:30 -0800 Subject: [PATCH 2/2] Clean up error messages --- lib/dartdoc.dart | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/lib/dartdoc.dart b/lib/dartdoc.dart index ee99d1b0c3..2c1057036d 100644 --- a/lib/dartdoc.dart +++ b/lib/dartdoc.dart @@ -140,7 +140,7 @@ class Dartdoc extends PackageBuilder { dartdocResults.packageGraph.packageWarningCounter.errorCount; if (errorCount > 0) { throw DartdocFailure( - "dartdoc encountered $errorCount} errors while processing."); + "dartdoc encountered $errorCount errors while processing."); } logInfo( 'Success! Docs generated into ${dartdocResults.outDir.absolute.path}');