Simplify LibraryContainer hierarchy and tests (#4121)

This one is a bit of a story; I could break it up if that would help the
review, but here goes:

1. I saw that LibraryContainer, an `abstract mixin class`, was (a)
_mixed into_ Category, and (b) _extended_ by Package. Weird. So I tried
to make it a _base_ class, a proper superclass of those two. Well that
showed me the two test classes: TestLibraryContainer and
TestLibraryContainerSdk.
2. What do those two test classes do? They existed for a mere 2 test
cases in `model_test.dart`. These test cases just test the `compareTo`
logic in LibraryContainer, but it is not terribly obvious how they test
them. The TestLibraryContainer seemed very generic, and odd, with its
own ContainerOrder list. In practice, Categories are always ordered by
the `--category-order` option, and Packages are always ordered by the
`--package-order` option. There's no need to test some generic ordering
system.
3. So, I was able to tear down those test classes and their two test
cases, and rewrite them in a more real-world test `group` in
`packages_test.dart`.
4. This also allows `isSdk` and `enclosingName` to be simple, final
fields on LibraryContainer, and `enclosingName` can be private. And then
`containerOrder` only needs to stay public in order to override.
5. This refactoring did require some fixes in the `writePackage` test
function: (a) it was writing a `.packages` file, and (b) it was writing
hard-coded package names into a package config file. We fix this by
passing in a list of dependency names.

Author: srawlins

lib/src/model/category.dart

@@ -11,13 +11,12 @@ import 'package:dartdoc/src/model/model.dart';
 import 'package:dartdoc/src/warnings.dart';
 
 /// A subcategory of a package, containing elements tagged with `{@category}`.
-class Category
+final class Category extends LibraryContainer
     with
         Nameable,
         Warnable,
         CommentReferable,
         MarkdownFileDocumentation,
-        LibraryContainer,
         TopLevelContainer
     implements Documentable {
   /// The package in which this category is contained.
@@ -64,7 +63,8 @@ class Category
   Category(this._name, this.package, this.config)
       : _categoryDefinition =
             config.categories.categoryDefinitions[_name.orDefault] ??
-                CategoryDefinition(_name, null, null);
+                CategoryDefinition(_name, null, null),
+        super(isSdk: false, enclosingName: package.name);
 
   Iterable<ExternalItem> get externalItems => _categoryDefinition.externalItems;
 
@@ -88,9 +88,6 @@ class Category
   @override
   List<String> get containerOrder => config.categoryOrder;
 
-  @override
-  String get enclosingName => package.name;
-
   @override
   PackageGraph get packageGraph => package.packageGraph;
 

lib/src/model/library_container.dart

@@ -5,12 +5,13 @@
 import 'package:collection/collection.dart';
 import 'package:dartdoc/src/model/model.dart';
 import 'package:dartdoc/src/model_utils.dart' as model_utils;
+import 'package:meta/meta.dart';
 
 /// A set of libraries, initialized after construction by accessing [libraries].
 ///
 /// Do not cache return values of any methods or members excepting [libraries]
 /// and [name] before finishing initialization of a [LibraryContainer].
-abstract mixin class LibraryContainer
+abstract base class LibraryContainer
     implements Nameable, Comparable<LibraryContainer>, Documentable {
   final List<Library> libraries = [];
 
@@ -19,31 +20,40 @@ abstract mixin class LibraryContainer
 
   bool get hasPublicLibraries => libraries.any((e) => e.isPublic);
 
+  LibraryContainer({required this.isSdk, required String enclosingName})
+      : _enclosingName = enclosingName;
+
   /// The name of the container or object that this LibraryContainer is a part
-  /// of.  Used for sorting in [containerOrder].
-  String get enclosingName;
+  /// of.
+  ///
+  /// Used for sorting in [containerOrder].
+  final String _enclosingName;
 
   /// Order by which this container should be sorted.
+  @visibleForOverriding
   List<String> get containerOrder;
 
-  /// Sorting key.  [containerOrder] should contain these.
+  /// Sorting key.
+  ///
+  /// [containerOrder] should contain these.
   String get sortKey => name;
 
-  /// Does this container represent the SDK?  This can be false for containers
-  /// that only represent a part of the SDK.
-  bool get isSdk => false;
+  /// Whether this container represents the Dart SDK.
+  ///
+  /// This can be false for containers that only represent a part of the SDK.
+  final bool isSdk;
 
   /// Returns:
   /// * -1 if this container is listed in [containerOrder].
-  /// * 0 if this container is named the same as the [enclosingName].
+  /// * 0 if this container is named the same as the [_enclosingName].
   /// * 1 if this container represents the SDK.
-  /// * 2 if this group has a name that contains the name [enclosingName].
+  /// * 2 if this group has a name that contains the name [_enclosingName].
   /// * 3 otherwise.
   int get _group {
     if (containerOrder.contains(sortKey)) return -1;
-    if (equalsIgnoreAsciiCase(sortKey, enclosingName)) return 0;
+    if (equalsIgnoreAsciiCase(sortKey, _enclosingName)) return 0;
     if (isSdk) return 1;
-    if (sortKey.toLowerCase().contains(enclosingName.toLowerCase())) return 2;
+    if (sortKey.toLowerCase().contains(_enclosingName.toLowerCase())) return 2;
     return 3;
   }
 

lib/src/model/package.dart

@@ -29,7 +29,7 @@ const String htmlBasePlaceholder = r'%%__HTMLBASE_dartdoc_internal__%%';
 
 /// A [LibraryContainer] that contains [Library] objects related to a particular
 /// package.
-class Package extends LibraryContainer
+final class Package extends LibraryContainer
     with Nameable, Warnable, CommentReferable {
   @override
   final String name;
@@ -70,7 +70,10 @@ class Package extends LibraryContainer
           packageGraph.config,
           packageGraph.resourceProvider.getFolder(packagePath),
           packageGraph.resourceProvider,
-        );
+        ),
+        super(
+            isSdk: packageMeta.isSdk,
+            enclosingName: packageGraph.defaultPackageName);
 
   @override
   bool get isCanonical => true;
@@ -183,9 +186,6 @@ class Package extends LibraryContainer
     return DocumentLocation.missing;
   }();
 
-  @override
-  String get enclosingName => packageGraph.defaultPackageName;
-
   String get filePath => 'index.html';
 
   @override
@@ -364,9 +364,6 @@ class Package extends LibraryContainer
       packageGraph.localPackages.isNotEmpty &&
       identical(packageGraph.localPackages.first, this);
 
-  @override
-  bool get isSdk => packageMeta.isSdk;
-
   final String packagePath;
 
   String get version => packageMeta.version;

lib/src/package_config_provider.dart

@@ -25,6 +25,8 @@ class FakePackageConfigProvider implements PackageConfigProvider {
   /// A mapping of package config search locations to configured packages.
   final _packageConfigData = <String, List<package_config.Package>>{};
 
+  /// Adds the package named [name] at [root] to the package config for
+  /// [location].
   void addPackageToConfigFor(String location, String name, Uri root) {
     _packageConfigData
         .putIfAbsent(location, () => [])

test/end2end/model_test.dart

@@ -9,7 +9,6 @@ import 'package:analyzer/dart/element/type.dart';
 import 'package:analyzer/source/line_info.dart';
 import 'package:async/async.dart';
 import 'package:collection/src/iterable_extensions.dart';
-import 'package:dartdoc/src/dartdoc_options.dart';
 import 'package:dartdoc/src/element_type.dart';
 import 'package:dartdoc/src/matching_link_result.dart';
 import 'package:dartdoc/src/model/attribute.dart';
@@ -36,66 +35,6 @@ Future<PackageGraph> get testPackageGraph async =>
         excludeLibraries: ['css', 'code_in_comments'],
         additionalArguments: ['--no-link-to-remote']));
 
-/// For testing sort behavior.
-class TestLibraryContainer extends LibraryContainer with Nameable {
-  @override
-  final List<String> containerOrder;
-  @override
-  String enclosingName;
-  @override
-  final String name;
-
-  @override
-  bool get isSdk => false;
-  @override
-  PackageGraph get packageGraph => throw UnimplementedError();
-
-  @override
-  Package get package => throw UnimplementedError();
-
-  TestLibraryContainer(
-      this.name, this.containerOrder, LibraryContainer? enclosingContainer)
-      : enclosingName = enclosingContainer?.name ?? '';
-
-  @override
-  DartdocOptionContext get config => throw UnimplementedError();
-
-  @override
-  String? get documentation => throw UnimplementedError();
-
-  @override
-  String get documentationAsHtml => throw UnimplementedError();
-
-  @override
-  bool get hasDocumentation => throw UnimplementedError();
-
-  @override
-  String? get href => throw UnimplementedError();
-
-  @override
-  bool get isDocumented => throw UnimplementedError();
-
-  @override
-  Kind get kind => throw UnimplementedError();
-
-  @override
-  String get oneLineDoc => throw UnimplementedError();
-
-  @override
-  String? get aboveSidebarPath => null;
-
-  @override
-  String? get belowSidebarPath => null;
-}
-
-class TestLibraryContainerSdk extends TestLibraryContainer {
-  TestLibraryContainerSdk(super.name, super.containerOrder,
-      LibraryContainer super.enclosingContainer);
-
-  @override
-  bool get isSdk => true;
-}
-
 void main() async {
   final packageGraph = await testPackageGraph;
   late final Library exLibrary;
@@ -737,64 +676,6 @@ void main() async {
     expect(category.redirectFilePath, 'topics/Superb-topic.html');
   });
 
-  group('LibraryContainer', () {
-    late final TestLibraryContainer topLevel;
-    var sortOrderBasic = ['theFirst', 'second', 'fruit'];
-    var containerNames = [
-      'moo',
-      'woot',
-      'theFirst',
-      'topLevel Things',
-      'toplevel',
-      'fruit'
-    ];
-
-    setUpAll(() {
-      topLevel = TestLibraryContainer('topLevel', [], null);
-    });
-
-    test('multiple containers with specified sort order', () {
-      var containers = <LibraryContainer>[];
-      for (var i = 0; i < containerNames.length; i++) {
-        var name = containerNames[i];
-        containers.add(TestLibraryContainer(name, sortOrderBasic, topLevel));
-      }
-      containers.add(TestLibraryContainerSdk('SDK', sortOrderBasic, topLevel));
-      containers.sort();
-      expect(
-          containers.map((c) => c.name),
-          orderedEquals([
-            'theFirst',
-            'fruit',
-            'toplevel',
-            'SDK',
-            'topLevel Things',
-            'moo',
-            'woot'
-          ]));
-    });
-
-    test('multiple containers, no specified sort order', () {
-      var containers = <LibraryContainer>[];
-      for (var name in containerNames) {
-        containers.add(TestLibraryContainer(name, [], topLevel));
-      }
-      containers.add(TestLibraryContainerSdk('SDK', [], topLevel));
-      containers.sort();
-      expect(
-          containers.map((c) => c.name),
-          orderedEquals([
-            'toplevel',
-            'SDK',
-            'topLevel Things',
-            'fruit',
-            'moo',
-            'theFirst',
-            'woot'
-          ]));
-    });
-  });
-
   group('Library', () {
     late final Library anonLib,
         isDeprecated,

test/packages_test.dart

@@ -277,8 +277,9 @@ library script;
 class Script {}
 ''');
 
-        packageTwoRoot =
-            utils.writePackage('two', resourceProvider, packageConfigProvider);
+        packageTwoRoot = utils.writePackage(
+            'two', resourceProvider, packageConfigProvider,
+            dependencies: ['one']);
         packageConfigProvider.addPackageToConfigFor(
             packageTwoRoot.path, 'one', Uri.file('${packageOneRoot.path}/'));
         packageTwoRoot
@@ -456,6 +457,64 @@ int x;
         expect(packageGraph.localPackages.first.categories, isEmpty);
       });
     });
+
+    group('ordering', () {
+      late Folder defaultRoot;
+
+      Folder bootBasicPackages(List<String> names) {
+        var defaultPackageRoot = utils.writePackage(
+            'default', resourceProvider, packageConfigProvider,
+            dependencies: names);
+
+        for (var name in names) {
+          var root =
+              utils.writePackage(name, resourceProvider, packageConfigProvider);
+          root
+              .getChildAssumingFolder('lib')
+              .getChildAssumingFile('$name.dart')
+              .writeAsStringSync('var a = 1;');
+          packageConfigProvider.addPackageToConfigFor(
+              defaultPackageRoot.path, name, Uri.file('${root.path}/'));
+          defaultPackageRoot
+              .getChildAssumingFolder('lib')
+              .getChildAssumingFile('$name.dart')
+              .writeAsStringSync('''
+import 'package:$name/$name.dart';
+var b = a;
+''');
+        }
+
+        return defaultPackageRoot;
+      }
+
+      test('default package precedes SDK which precedes dependencies',
+          () async {
+        defaultRoot = bootBasicPackages(['bbb', 'aaa']);
+        var packageGraph = await utils.bootBasicPackage(
+            defaultRoot.path, packageMetaProvider, packageConfigProvider,
+            additionalArguments: [
+              '--auto-include-dependencies',
+            ]);
+
+        var sortedPackages = [...packageGraph.localPackages]..sort();
+        expect(sortedPackages.map((c) => c.name),
+            orderedEquals(['default', 'Dart', 'aaa', 'bbb']));
+      });
+
+      test('packages named in --package-order option come first', () async {
+        defaultRoot = bootBasicPackages(['aaa', 'bbb', 'ccc']);
+        var packageGraph = await utils.bootBasicPackage(
+            defaultRoot.path, packageMetaProvider, packageConfigProvider,
+            additionalArguments: [
+              '--auto-include-dependencies',
+              '--package-order=bbb,aaa',
+            ]);
+
+        var sortedPackages = [...packageGraph.localPackages]..sort();
+        expect(sortedPackages.map((c) => c.name),
+            orderedEquals(['bbb', 'aaa', 'default', 'Dart', 'ccc']));
+      });
+    });
   });
 }