Rearrange SDK PackageMeta instantiation to be similar to normal packages (#1639)

* Fix or disable miscellaneous lints in dartdoc

* Clean up a few more lints.

* Rename category to package in all remaining references

* dartfmt

* Review comments

* Intermediate state

* basic dartdoc options now working

* Fix test failure (accidentally caching config info)

* dartfmt

* dartfmt

* one last rename

* Remove overly complicated config inheritance; we can add it back if we truly need this

* dartfmt

* Unify SDK PackageMeta construction and add cache

* update test package docs

* PackageMeta construction sufficiently robust for refactor

* Fix infinite loop and error messages

* Rename path/p import directive to pathLib everywhere.

* dartfmt

* 'Dart SDK' + '' => 'Dart Core' + 'SDK'

* SDK name is no longer special

Author: jcollins-g

bin/dartdoc.dart

@@ -52,14 +52,15 @@ main(List<String> arguments) async {
   final bool sdkDocs = args['sdk-docs'];
   final bool showProgress = args['show-progress'];
 
-  final String readme = args['sdk-readme'];
-  if (readme != null && !(new File(readme).existsSync())) {
-    stderr.writeln(
-        " fatal error: unable to locate the SDK description file at $readme.");
-    exit(1);
+  Directory inputDir;
+  if (sdkDocs) {
+    inputDir = sdkDir;
+  } else if (args['input'] == null) {
+    inputDir = Directory.current;
+  } else {
+    inputDir = args['input'];
   }
 
-  Directory inputDir = new Directory(args['input']);
   if (!inputDir.existsSync()) {
     stderr.writeln(
         " fatal error: unable to locate the input directory at ${inputDir
@@ -188,12 +189,12 @@ main(List<String> arguments) async {
     });
   }
 
-  PackageMeta packageMeta = sdkDocs
-      ? new PackageMeta.fromSdk(sdkDir,
-          sdkReadmePath: readme,
-          displayAsPackages:
-              args['use-categories'] || args['display-as-packages'])
-      : new PackageMeta.fromDir(inputDir);
+  PackageMeta packageMeta = new PackageMeta.fromDir(inputDir);
+
+  if (packageMeta == null) {
+    stderr.writeln(' fatal error: Unable to generate documentation: no pubspec.yaml found');
+    exit(1);
+  }
 
   if (!packageMeta.isValid) {
     final String firstError = packageMeta.getInvalidReasons().first;
@@ -211,6 +212,7 @@ main(List<String> arguments) async {
     }
   }
 
+
   logInfo("Generating documentation for '${packageMeta}' into "
       "${outputDir.absolute.path}${Platform.pathSeparator}");
 
@@ -310,9 +312,9 @@ ArgParser _createArgsParser() {
       help: 'Display progress indications to console stdout', negatable: false);
   parser.addOption('sdk-readme',
       help:
-          'Path to the SDK description file; use if generating Dart SDK docs.');
+          'Path to the SDK description file.  Deprecated (ignored)');
   parser.addOption('input',
-      help: 'Path to source directory.', defaultsTo: Directory.current.path);
+      help: 'Path to source directory.');
   parser.addOption('output',
       help: 'Path to output directory.', defaultsTo: defaultOutDir);
   parser.addMultiOption('header',

lib/src/model.dart

@@ -1710,7 +1710,6 @@ class Library extends ModelElement {
   List<TopLevelVariable> _variables;
   Namespace _exportedNamespace;
   String _name;
-  String _packageName;
   factory Library(LibraryElement element, PackageGraph packageGraph) {
     return packageGraph.findOrCreateLibraryFor(element);
   }
@@ -2015,16 +2014,7 @@ class Library extends ModelElement {
 
   /// The real package, as opposed to the package we are documenting it with,
   /// [PackageGraph.name]
-  String get packageName {
-    if (_packageName == null) {
-      if (name.startsWith('dart:')) {
-        _packageName = 'Dart Core';
-      } else {
-        _packageName = packageMeta?.name ?? '';
-      }
-    }
-    return _packageName;
-  }
+  String get packageName => packageMeta?.name ?? '';
 
   /// The real packageMeta, as opposed to the package we are documenting with.
   PackageMeta _packageMeta;
@@ -2187,16 +2177,7 @@ class Library extends ModelElement {
 
   static PackageMeta getPackageMeta(LibraryElement element) {
     String sourcePath = element.source.fullName;
-    File file = new File(pathLib.canonicalize(sourcePath));
-    Directory dir = file.parent;
-    while (dir.parent.path != dir.path && dir.existsSync()) {
-      File pubspec = new File(pathLib.join(dir.path, 'pubspec.yaml'));
-      if (pubspec.existsSync()) {
-        return new PackageMeta.fromDir(dir);
-      }
-      dir = dir.parent;
-    }
-    return null;
+    return new PackageMeta.fromDir(new File(pathLib.canonicalize(sourcePath)).parent);
   }
 
   static String getLibraryName(LibraryElement element) {
@@ -4204,7 +4185,7 @@ class PackageGraph extends Canonicalization with Nameable, Warnable {
   String get name => packageMeta.name;
 
   String get kind =>
-      (packageMeta.displayAsPackages || packageGraph.isSdk) ? '' : 'package';
+      (packageGraph.isSdk) ? 'SDK' : 'package';
 
   @override
   String get oneLineDoc => '';
@@ -4543,14 +4524,14 @@ class Package implements Comparable<Package> {
   /// Returns:
   /// -1 if this package is listed in --package-order.
   /// 0 if this package is the original package we are documenting.
-  /// 1 if this group represents the Dart SDK.
-  /// 2 if this group has a name that contains the name of the original
+  /// 1 if this package represents the Dart SDK.
+  /// 2 if this package has a name that contains the name of the original
   ///   package we are documenting.
   /// 3 otherwise.
   int get _group {
     if (config.packageOrder.contains(name)) return -1;
     if (name.toLowerCase() == packageGraph.name.toLowerCase()) return 0;
-    if (name == "Dart Core") return 1;
+    if (isSdk) return 1;
     if (name.toLowerCase().contains(packageGraph.name.toLowerCase())) return 2;
     return 3;
   }

lib/src/package_meta.dart

@@ -6,22 +6,49 @@ library dartdoc.package_meta;
 
 import 'dart:io';
 
+import 'package:dartdoc/src/sdk.dart';
 import 'package:path/path.dart' as pathLib;
 import 'package:yaml/yaml.dart';
 
 import 'logging.dart';
 
+
+Map<String, PackageMeta> _packageMetaCache = {};
+
 abstract class PackageMeta {
   final Directory dir;
-  final bool displayAsPackages;
-
-  PackageMeta(this.dir, {this.displayAsPackages: false});
 
-  factory PackageMeta.fromDir(Directory dir) => new _FilePackageMeta(dir);
-  factory PackageMeta.fromSdk(Directory sdkDir,
-          {String sdkReadmePath, bool displayAsPackages}) =>
-      new _SdkMeta(sdkDir,
-          sdkReadmePath: sdkReadmePath, displayAsPackages: displayAsPackages);
+  PackageMeta(this.dir);
+
+  /// This factory is guaranteed to return the same object for any given
+  /// [dir.absolute.path].  Multiple [dir.absolute.path]s will resolve to the
+  /// same object if they are part of the same package.  Returns null
+  /// if the directory is not part of a known package.
+  factory PackageMeta.fromDir(Directory dir) {
+    Directory original = dir.absolute;
+    dir = original;
+    if (!_packageMetaCache.containsKey(dir.path)) {
+      PackageMeta packageMeta;
+      // There are pubspec.yaml files inside the SDK.  Ignore them.
+      if (pathLib.isWithin(getSdkDir().absolute.path, dir.path) || getSdkDir().path == dir.path) {
+        packageMeta = new _SdkMeta(getSdkDir());
+      } else {
+        while (dir.existsSync()) {
+          File pubspec = new File(pathLib.join(dir.path, 'pubspec.yaml'));
+          if (pubspec.existsSync()) {
+            packageMeta = new _FilePackageMeta(dir);
+            break;
+          }
+          // Allow a package to be at root (possible in a Windows setting with
+          // drive letter mappings).
+          if (dir.path == dir.parent.absolute.path) break;
+          dir = dir.parent.absolute;
+        }
+      }
+      _packageMetaCache[dir.absolute.path] = packageMeta;
+    }
+    return _packageMetaCache[dir.absolute.path];
+  }
 
   bool get isSdk;
   bool get needsPubGet => false;
@@ -46,7 +73,7 @@ abstract class PackageMeta {
   FileContents getChangelogContents();
 
   /// Returns true if we are a valid package, valid enough to generate docs.
-  bool get isValid;
+  bool get isValid => getInvalidReasons().isEmpty;
 
   /// Returns a list of reasons this package is invalid, or an
   /// empty list if no reasons found.
@@ -151,8 +178,6 @@ class _FilePackageMeta extends PackageMeta {
     return _changelog;
   }
 
-  @override
-  bool get isValid => getInvalidReasons().isEmpty;
 
   /// Returns a list of reasons this package is invalid, or an
   /// empty list if no reasons found.
@@ -184,10 +209,12 @@ File _locate(Directory dir, List<String> fileNames) {
 }
 
 class _SdkMeta extends PackageMeta {
-  final String sdkReadmePath;
+  String sdkReadmePath;
 
-  _SdkMeta(Directory dir, {this.sdkReadmePath, bool displayAsPackages})
-      : super(dir, displayAsPackages: displayAsPackages);
+  _SdkMeta(Directory dir)
+      : super(dir) {
+    sdkReadmePath = pathLib.join(dir.path, 'lib', 'api_readme.md');
+  }
 
   @override
   bool get isSdk => true;
@@ -198,7 +225,7 @@ class _SdkMeta extends PackageMeta {
   }
 
   @override
-  String get name => 'Dart SDK';
+  String get name => 'Dart';
   @override
   String get version =>
       new File(pathLib.join(dir.path, 'version')).readAsStringSync().trim();
@@ -211,15 +238,10 @@ class _SdkMeta extends PackageMeta {
 
   @override
   FileContents getReadmeContents() {
-    File f = sdkReadmePath != null
-        ? new File(sdkReadmePath)
-        : new File(pathLib.join(dir.path, 'lib', 'api_readme.md'));
+    File f = new File(pathLib.join(dir.path, 'lib', 'api_readme.md'));
     return f.existsSync() ? new FileContents(f) : null;
   }
 
-  @override
-  bool get isValid => true;
-
   @override
   List<String> getInvalidReasons() => [];
 

lib/templates/index.html

@@ -1,7 +1,7 @@
 {{>head}}
 
   <div class="col-xs-6 col-sm-3 col-md-2 sidebar sidebar-offcanvas-left">
-    <h5>{{self.name}} {{self.kind}}</h5>
+    <h5><span class="package-name">{{self.name}}</span> <span class="package-kind">{{self.kind}}</span></h5>
 
     {{#displayAsPackages}}
     <ol>
@@ -11,7 +11,6 @@ <h5>{{self.name}} {{self.kind}}</h5>
         <li>{{{linkedName}}}</li>
         {{/publicLibraries}}
       {{/packageGraph.publicPackages}}
-
     </ol>
     {{/displayAsPackages}}
 

lib/templates/library.html

@@ -1,7 +1,7 @@
 {{>head}}
 
   <div class="col-xs-6 col-sm-3 col-md-2 sidebar sidebar-offcanvas-left">
-    <h5>{{parent.name}} {{parent.kind}}</h5>
+    <h5><span class="package-name">{{parent.name}}</span> <span class="package-kind">{{parent.kind}}</span></h5>
     {{#displayAsPackages}}
     <ol>
       {{#packageGraph.publicPackages}}

test/model_test.dart

@@ -104,7 +104,8 @@ void main() {
       });
 
       test('sdk name', () {
-        expect(sdkAsPackageGraph.name, equals('Dart SDK'));
+        expect(sdkAsPackageGraph.name, equals('Dart'));
+        expect(sdkAsPackageGraph.kind, equals('SDK'));
       });
 
       test('sdk homepage', () {

test/package_meta_test.dart

@@ -8,26 +8,19 @@ import 'dart:io';
 
 import 'package:dartdoc/src/package_meta.dart';
 import 'package:dartdoc/src/sdk.dart';
-import 'package:path/path.dart' as pathLib;
 import 'package:test/test.dart';
 
 void main() {
   group('PackageMeta for a directory without a pubspec', () {
     PackageMeta p;
 
     setUp(() {
-      var d = new Directory(pathLib.join(
-          Directory.current.path, 'testing/test_package_not_valid'));
-      if (!d.existsSync()) {
-        throw "$d cannot be found";
-      }
+      var d = Directory.systemTemp.createTempSync('test_package_not_valid');
       p = new PackageMeta.fromDir(d);
     });
 
     test('is not valid', () {
-      expect(p.isValid, isFalse);
-      expect(p.getInvalidReasons(), isNotEmpty);
-      expect(p.getInvalidReasons(), contains('no pubspec.yaml found'));
+      expect(p, isNull);
     });
   });
 
@@ -76,15 +69,16 @@ void main() {
   });
 
   group('PackageMeta.fromSdk', () {
-    PackageMeta p = new PackageMeta.fromSdk(getSdkDir());
+    PackageMeta p = new PackageMeta.fromDir(getSdkDir());
 
     test('has a name', () {
-      expect(p.name, 'Dart SDK');
+      expect(p.name, 'Dart');
     });
 
     test('is valid', () {
       expect(p.isValid, isTrue);
       expect(p.getInvalidReasons(), isEmpty);
+      expect(p.isSdk, isTrue);
     });
 
     test('has a version', () {

test/src/utils.dart

@@ -34,7 +34,7 @@ void delete(Directory dir) {
 
 init() async {
   sdkDir = getSdkDir();
-  sdkPackageMeta = new PackageMeta.fromSdk(sdkDir);
+  sdkPackageMeta = new PackageMeta.fromDir(sdkDir);
   setConfig();
 
   testPackageGraph = await bootBasicPackage(

testing/test_package_docs/anonymous_library/anonymous_library-library.html

@@ -35,7 +35,7 @@
 <main>
 
   <div class="col-xs-6 col-sm-3 col-md-2 sidebar sidebar-offcanvas-left">
-    <h5>test_package package</h5>
+    <h5><span class="package-name">test_package</span> <span class="package-kind">package</span></h5>
 
     <ol>
       <li class="section-title"><a href="index.html#libraries">Libraries</a></li>

testing/test_package_docs/another_anonymous_lib/another_anonymous_lib-library.html

@@ -35,7 +35,7 @@
 <main>
 
   <div class="col-xs-6 col-sm-3 col-md-2 sidebar sidebar-offcanvas-left">
-    <h5>test_package package</h5>
+    <h5><span class="package-name">test_package</span> <span class="package-kind">package</span></h5>
 
     <ol>
       <li class="section-title"><a href="index.html#libraries">Libraries</a></li>