Canonicalization overhaul part two #1390

jcollins-g · 2017-05-01T20:38:23Z

Welcome to the second part of the canonicalization overhaul begun in #1368. It's somewhat smaller and less fundamental.

The key elements are the rewrite of how comment references are handled (changes to _findRefElementInLibrary and its callers particularly) and the new hints to ModelElement.from and findCanonicalModelElementFor -- if you understand those, most of the remaining structural changes are understandable.

The link checker and warning enhancements are, in retrospect, somewhat orthogonal to the change, but were very much necessary to be able to corral all the consequences from canonicalization and insure they remain fixed.

More unit tests are on deck for some of the features and capabilities the code has now, but since the many new asserts and the new, better warnings make problems much more visible, regressions will be detectable in other ways. Given that and upcoming flutter deadlines, I wanted to get this out for review ASAP.

devoncarew · 2017-05-02T14:46:19Z

.idea/modules.xml

@@ -3,6 +3,7 @@
  <component name="ProjectModuleManager">
    <modules>
      <module fileurl="file://$PROJECT_DIR$/dartdoc.iml" filepath="$PROJECT_DIR$/dartdoc.iml" />
+      <module fileurl="file://$PROJECT_DIR$/../mustache4dart/mustache4dart.iml" filepath="$PROJECT_DIR$/../mustache4dart/mustache4dart.iml" />


probably want to drop this line

I would prefer to just delete the whole file so I can have my own intellij settings. Is that a bad idea?

No, feel free to delete.

devoncarew · 2017-05-02T14:47:21Z

CHANGELOG.md

+  * Upgrades to new Package.warn system.
+    * Fully integrated all scattered "warnings" and added new ones for the link checker.
+    * Allow for setting which warnings are errors in the library.
+    * Change location output to something IntelliJ can understand and link to


jcollins-g · 2017-05-02T15:20:45Z

Travis failures are because for some reason the link checker allocates a tremendous amount of memory. I'll try to figure out why that is.

devoncarew

Nice changes! I made one pass through.

You alluded to this in your PR comments, but some of the changes here - the warnings overhaul for example - could have been handled in separate, smaller PRs before this one. That would make for happier reviewers - it's hard to meaningfully review PRs of this size.

I note that the flutter bot is failing on this PR; do you know why?

Travis failures are because for some reason the link checker allocates a tremendous amount of memory. I'll try to figure out why that is.

Nevermind, looks like our comments crossed.

devoncarew · 2017-05-02T14:51:25Z

CHANGELOG.md

+
+* Many cleanups to dartdoc stdout/stderr, error messages, and warnings:
+  * Display fatal errors with 'fatal error' string to distinguish them from ordinary errors
+  * Upgrades to new Package.warn system.


Food for thought for later - Dart now has good support for detecting when a console supports ansi. There are some very lightweight libraries for colorizing console output and showing indeterminate progress. We may want to use ansi colors when writing warnings and errors.

https://github.com/google/tuneup.dart/blob/master/lib/src/ansi.dart

https://github.com/google/tuneup.dart/blob/master/lib/src/logger.dart#L92

Nice. I was wondering about that.

devoncarew · 2017-05-02T14:58:17Z

lib/dartdoc.dart

@@ -167,21 +185,170 @@ class DartDoc {

    for (var generator in generators) {
      await generator.generate(package, outputDir);
+      generator.writtenFiles


Perhaps slightly clearer as writtenFiles.addAll(generator.writtenFiles.map(path.normalize))?

devoncarew · 2017-05-02T15:01:04Z

lib/dartdoc.dart

+    String staticAssets = path.joinAll([normalOrigin, 'static-assets', '']);
+    String indexJson = path.joinAll([normalOrigin, 'index.json']);
+    bool foundIndex = false;
+    await for (FileSystemEntity f


Perhaps a bit simpler to use listSync()?

I think you could then remove the async from _doOrphanCheck.

And - not necessarily for this PR - it would be interesting to add the lint that checks that you're only awaiting Futures. It should then fire when you make _doOrphanCheck sync, on the await in verifyLinks.

Sounds good. Went with listSync and removed async where appropriate.

devoncarew · 2017-05-02T15:04:06Z

lib/dartdoc.dart

+      fullPath = path.normalize(fullPath);
+    }
+
+    File file = new File("$fullPath");


new File(fullPath)?

devoncarew · 2017-05-02T15:04:44Z

lib/dartdoc.dart

+    if (!file.existsSync()) {
+      _warn(package, PackageWarning.brokenLink, pathToCheck,
+          path.normalize(origin),
+          source: source);


This may auto-format slightly nicer if you add a trailing comma after source.

devoncarew · 2017-05-02T15:28:25Z

lib/src/model.dart

@@ -828,7 +879,7 @@ class Constructor extends ModelElement
    String constructorName = element.name;
    Class c = new ModelElement.from(element.enclosingElement, library) as Class;
    if (constructorName.isEmpty) {
-      return c.name;
+      return '${c.name}';


Is name a String? This isn't necessary, unless you don't want to return null.

Yeah, no need for this here, there were intermediate changes I had that renamed all constructors to "ClassName.ConstructorName", even defaults. Fixed.

devoncarew · 2017-05-02T15:30:16Z

lib/src/model.dart

+  }
+
+  String get arrow {
+    if (readOnly) return r'&#8594;';


You could have the actual char in a line comment to help readers (// →).

devoncarew · 2017-05-02T15:33:03Z

lib/src/model.dart

-class Package implements Nameable, Documentable {
+/// Provides description text and command line flags for warnings.
+/// TODO(jcollins-g): Actually use this for command line flags.
+Map<PackageWarning, List<String>> packageWarningText = {


Very cool! Nice to have a centralized structure for the errors we emit.

devoncarew · 2017-05-02T15:36:54Z

lib/src/model.dart

+  PackageWarningOptions() {
+    _asWarnings.addAll(PackageWarning.values);
+    ignore(PackageWarning.typeAsHtml);
+    //error(PackageWarning.brokenLink);


devoncarew · 2017-05-02T15:38:18Z

lib/src/model.dart

+            'dartdoc detected an unknown file in the doc tree:  ${message}';
+        break;
+      case PackageWarning.typeAsHtml:
+        warningMessage = 'generic type handled as HTML: """${message}"""';


Why so many quotes? A single " should work here too.

I found it visually indistinct when the sample message often contained quotes and so on for this warning.

Added clarifying comment, but it turned out that the symbols typically involved in the message string made it visually indistinct to use single quotes.

jcollins-g · 2017-05-02T16:12:47Z

Yes. I actually tried to avoid fixing the warnings while fixing the broken links, but incrementally I found it impossible to correct the broken links without doing so, so the two wound up happening together. Same with the link checker.

I expect there to be a finite, small number of these giant PRs. I think the code is improved enough now that future PRs will not be wide ranging; a goal here is to make it less structurally necessary to touch quite so much just to fix something like #1391.

jcollins-g · 2017-05-02T16:24:47Z

.idea/modules.xml

@@ -3,6 +3,7 @@
  <component name="ProjectModuleManager">
    <modules>
      <module fileurl="file://$PROJECT_DIR$/dartdoc.iml" filepath="$PROJECT_DIR$/dartdoc.iml" />
+      <module fileurl="file://$PROJECT_DIR$/../mustache4dart/mustache4dart.iml" filepath="$PROJECT_DIR$/../mustache4dart/mustache4dart.iml" />


jcollins-g · 2017-05-02T16:25:30Z

CHANGELOG.md

+
+* Many cleanups to dartdoc stdout/stderr, error messages, and warnings:
+  * Display fatal errors with 'fatal error' string to distinguish them from ordinary errors
+  * Upgrades to new Package.warn system.


Nice. I was wondering about that.

jcollins-g · 2017-05-02T16:26:06Z

lib/dartdoc.dart

@@ -167,21 +185,170 @@ class DartDoc {

    for (var generator in generators) {
      await generator.generate(package, outputDir);
+      generator.writtenFiles


jcollins-g · 2017-05-02T16:26:40Z

lib/dartdoc.dart

+    String staticAssets = path.joinAll([normalOrigin, 'static-assets', '']);
+    String indexJson = path.joinAll([normalOrigin, 'index.json']);
+    bool foundIndex = false;
+    await for (FileSystemEntity f


Sounds good. Went with listSync and removed async where appropriate.

jcollins-g · 2017-05-02T16:27:52Z

lib/dartdoc.dart

+    _hrefs = package.allHrefs;
+
+    Set<String> visited = new Set();
+    String start = 'index.html';


jcollins-g · 2017-05-02T17:13:48Z

lib/src/model.dart

+  PackageWarningOptions() {
+    _asWarnings.addAll(PackageWarning.values);
+    ignore(PackageWarning.typeAsHtml);
+    //error(PackageWarning.brokenLink);


jcollins-g · 2017-05-02T17:15:28Z

lib/src/model.dart

+            'dartdoc detected an unknown file in the doc tree:  ${message}';
+        break;
+      case PackageWarning.typeAsHtml:
+        warningMessage = 'generic type handled as HTML: """${message}"""';


I found it visually indistinct when the sample message often contained quotes and so on for this warning.

jcollins-g · 2017-05-02T17:36:18Z

lib/src/model.dart

+  }
+
+  String get arrow {
+    if (readOnly) return r'&#8594;';


jcollins-g · 2017-05-02T17:37:18Z

lib/src/model.dart

+  PackageWarningOptions() {
+    _asWarnings.addAll(PackageWarning.values);
+    ignore(PackageWarning.typeAsHtml);
+    //error(PackageWarning.brokenLink);


jcollins-g · 2017-05-02T17:38:41Z

lib/src/model.dart

+            'dartdoc detected an unknown file in the doc tree:  ${message}';
+        break;
+      case PackageWarning.typeAsHtml:
+        warningMessage = 'generic type handled as HTML: """${message}"""';


Added clarifying comment, but it turned out that the symbols typically involved in the message string made it visually indistinct to use single quotes.

devoncarew · 2017-05-03T20:07:11Z

CHANGELOG.md

+    * Change --show-warnings to show all warnings, even those that might not be useful yet.
+  * Display a count of all warnings/errors after document generation.
+  * Make the progress counter tick slower.
+* Added a built-in link checker and orphaned file generator, and tied it into Package.warn so


==> orphaned file checker?

devoncarew · 2017-05-03T21:05:48Z

lgtm for my part! Excited to get this into play for the flutter docs

jcollins-g · 2017-05-03T20:35:55Z

lib/src/markdown_processor.dart

+// TODO(jcollins-g): Rewrite this to handle constructors in a less hacky way
+// TODO(jcollins-g): This function breaks down naturally into many helpers, extract them
+// TODO(jcollins-g): Subcomponents of this function shouldn't be adding nulls to results, strip the
+//                   removes out that are gratuitous and


missing end of comment

jcollins-g · 2017-05-03T20:37:10Z

lib/src/markdown_processor.dart

+// and will add to [results]
+void _getResultsForClass(Class tryClass, String codeRefChomped,
+    Set<ModelElement> results, String codeRef, Package package) {
+  if ((tryClass.modelType.typeArguments.map((e) => e.name))


comment, indicate this is for type arguments

jcollins-g · 2017-05-04T14:41:30Z

CHANGELOG.md

+    * Change --show-warnings to show all warnings, even those that might not be useful yet.
+  * Display a count of all warnings/errors after document generation.
+  * Make the progress counter tick slower.
+* Added a built-in link checker and orphaned file generator, and tied it into Package.warn so


jcollins-g added 3 commits May 1, 2017 09:51

Squash canonicalization overhaul part2 into single commit

a36caba

Cleanup and add comments

00769a5

dartfmt

dd0c072

jcollins-g requested review from devoncarew and keertip May 1, 2017 20:38

googlebot added the cla: yes Google CLA check succeeded. label May 1, 2017

devoncarew reviewed May 2, 2017

View reviewed changes

jcollins-g added 2 commits May 2, 2017 08:48

Update changelog, reduce memory usage for link checker

09706d8

dartfmt

48f30cd

delete dartdoc.iml

ef98fc1

This was referenced May 2, 2017

Change the order of groups on dartdoc pages #1323

Closed

New dartdoc warnings flutter/flutter#9726

Closed

jcollins-g added 2 commits May 2, 2017 11:16

Review comments

a009d9d

dartfmt

bc887df

jcollins-g commented May 2, 2017

View reviewed changes

whitespace in changelog

c31115e

devoncarew reviewed May 3, 2017

View reviewed changes

jcollins-g added 2 commits May 4, 2017 07:32

Comment enhancements

93b97a9

Correct changelog to 'orphaned file checker'

5efd477

jcollins-g commented May 4, 2017

View reviewed changes

update test package docs -- whitespace change in SDK

9aa79d1

jcollins-g merged commit 7764f87 into master May 4, 2017

This was referenced May 4, 2017

dartdoc should use analyzer to resolve ambiguity in commentRef resolution #1397

Closed

dartdoc needs a better defined namespace lookup #1414

Closed

This was referenced May 12, 2017

dartdoc's unit test setup diverges significantly from actual usage #1420

Closed

[new ui.Gradient.linear] doesn't linkify #1402

Closed

DartDoc for Dart SDK shows internal interfaces. #1173

Closed

jcollins-g deleted the canonicalization-overhaul-part2 branch May 15, 2017 20:37

jcollins-g mentioned this pull request Jun 13, 2017

Canonicalization scorer #1455

Merged

jcollins-g mentioned this pull request Oct 2, 2017

UnmodifiableListView extends class that is not part of Dart API #1481

Closed

jcollins-g mentioned this pull request Oct 18, 2017

canonicalization overhaul part 3: Types, Templates, and Public/Private #1524

Merged

Canonicalization overhaul part two #1390

Canonicalization overhaul part two #1390

Conversation

jcollins-g commented May 1, 2017 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jcollins-g commented May 2, 2017

devoncarew left a comment • edited Loading

Choose a reason for hiding this comment

devoncarew May 2, 2017 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jcollins-g commented May 2, 2017

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

devoncarew commented May 3, 2017

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jcollins-g commented May 1, 2017 •

edited

Loading

devoncarew left a comment •

edited

Loading

devoncarew May 2, 2017 •

edited

Loading