Canonicalization scorer #1455

jcollins-g · 2017-06-02T22:30:17Z

Fixes #1370. This is a new canonicalization scorer to enable dartdoc to make good decisions for where to put docs for a reexported element. The scorer uses paths much like @Hixie suggested in the fifth bullet point of this comment, with some additions (variants on the rest of that comment are already implemented). It eliminates all but 4 of the ambiguous reexport warnings from angular2, dropping the number of ambiguous reexport warnings by 97%. The last few can be fixed with a new directive, @canonicalFor, that can be used to disambiguate or override bad scorer decisions.

I did manually go through the hundred and thirty three warnings this eliminated and am convinced a human would agree with me as to where they should be canonical -- the last few that display I wasn't completely sure of. The flag --ambiguous-reexport-scorer-min-confidence will allow you to adjust when these warnings appear if you think the scorer is broken.

Old warnings:

.... and they scroll off the screen.

New warnings:

To support testing the @canonicalFor feature I had to make minor changes elsewhere in dartdoc. Additionally, the warning system has been enhanced to permit enhanced warning descriptions -- the ambiguous reexport warning is particularly challenging to understand and I felt without some more infrastructure to help the user even the smaller number of warnings wouldn't be actionable.

@chalin @kwalrath

…zation-scorer

kwalrath · 2017-06-02T22:38:28Z

Can't wait to see how this improves the Angular docs!

Hixie · 2017-06-02T22:53:39Z

oooOOOooo!

Do we need to use @canonicalFor anywhere in Flutter? What does it do? :-)

chalin · 2017-06-03T00:21:03Z

Very nice! I'm also looking forward to see this in action.

devoncarew

Not a review, just a pass through.

Really great news about the scoring improvements, esp. the drop in angulardart ambiguity!

Does this reduce the number of canonicalization warnings from the sdk when generating docs for say dartdoc itself? I suspect that these canonicalization improvements should eliminate most of those.

devoncarew · 2017-06-05T14:00:41Z

bin/dartdoc.dart

  DartSdk sdk = new FolderBasedDartSdk(PhysicalResourceProvider.INSTANCE,
      PhysicalResourceProvider.INSTANCE.getFolder(sdkDir.path));

  setConfig(
-      addCrossdart: addCrossdart,
+      addCrossdart: args['add-crossdart'] as bool,


I'm curious if the analyzer needs this casting - do you get a warning without the as bool?

devoncarew · 2017-06-05T14:02:03Z

bin/dartdoc.dart

@@ -266,6 +266,14 @@ ArgParser _createArgsParser() {
          "Generates `index.json` with indentation and newlines. The file is larger, but it's also easier to diff.",
      negatable: false,
      defaultsTo: false);
+  parser.addOption('ambiguous-reexport-scorer-min-confidence',


This seems like a super long name for a flag. Is this even a flag that user's should toggle? Should we just decide on a good value for this?

If you need a flag for dev purposes, you can have a hidden option with the hide: true param.

Added hide: true.

devoncarew · 2017-06-05T14:09:27Z

lib/src/model.dart

+      "ambiguous-reexport",
+      "A symbol is exported from private to public in more than one library and dartdoc can not determine which one is canonical",
+      [
+        "Use {@canonicalFor @@name@@} in the desired library's documentation to resolve",


@canonicalDeclarationFor? @canonicalDocLocation? @docLocationFor? @canonicalLocationFor? @dartdocCanonical?

I picked @canonicalFor since the tag is to appear in the library which is canonical for the symbol given as a parameter. It'll look like this:

/// Some docs for foo. /// {@canonicalFor bar.Baz} library foo;

Since we're not specifying a location as a parameter to the tag I tend to drop out options 2-4. I'm not sure what the first one would mean in this context. The last one seems to add unnecessary information (AFAIK there are no similar comment tags for other tools).

jcollins-g · 2017-06-05T16:47:46Z

--auto-include-deps was broken and I didn't notice it until the upload (we added src/mylib from the test package). I have no idea how it managed to work before, it probably was accidental. So, added a checker to isPrivate to never declare public something in lib/src of a package.

jcollins-g · 2017-06-05T18:22:04Z

@Hixie: The only ambiguous reexport warning in flutter is for Matrix4, since it is reexported from other packages directly. Flutter is constructed in such a way that it never crosses the private/public export boundary in more than one public library for a given symbol, so the canonicalization code knows to prioritize the library where the boundary was crossed and ignore other reexports.

For example:
lib/reexporter_of_foo.dart <--- lib/foo.dart <--- lib/src/my_private_library.dart
lib/another_reexporter_of_foo.dart <--- lib/foo.dart

As long as your reexports are structured this way, dartdoc knows to prioritize lib/foo.dart for symbols originally from lib/src/my_private_library.dart. Both the Dart SDK and flutter seem to be built this way.

Angular2 has a lot of things that look like this (simplified):

lib/foo.dart <--- lib/src/my_private_library.dart
lib/library.dart <--- lib/src/my_private_library.dart
lib/another_library.dart <--- lib/src/my_private_library.dart
lib/stuff_and_things.dart <--- lib/src/my_private_library.dart

With this structure, dartdoc used to just give up and pick one of these arbitrarily as canonical, emitting a warning. The scorer enables it to guess better. It sets values to things from the library names and directory structure, whether a library is deprecated, and so on, to determine where it should be canonical. But it's possible to guess wrong, and @canonicalFor makes up the difference there.

pq · 2017-06-05T21:10:21Z

lib/src/model.dart

+  /// documented with this library, but these ModelElements and names correspond
+  /// to the defining library where each originally came from with respect
+  /// to inheritance and reexporting.  Most useful for error reporting.
+  Iterable<String> get allOriginalModelElementNames {


Curious why you're not returning a List here. Is this to prevent write access?

If I'm not actually using the full interface I tend to try to limit what I return to what I need, in this case, Iterable. Still learning interfaces in Dart so I'm not doing that super consistently yet, but working on it.

pq · 2017-06-05T21:12:32Z

lib/src/model.dart

+  }
+
+  @override
+  String toString() {


Short form?

=> "${library.name}: ${score.toStringAsPrecision(4)} - ${reasons.join(', ')}";

pq

A few comments but nothing substantial. (And hey it's hard to argue with the results; a 97% drop ain't bad!) Thanks for the context and screen shot. Much appreciated!

👍

pq · 2017-06-05T21:13:39Z

lib/src/model.dart

+          .where((s) => s.isNotEmpty));
+  }
+
+  Set<String> get namePieces {


Similarly here. I prefer the => form but I guess it's a matter of taste. 🤔

Done, and extracted into Nameable.

pq · 2017-06-05T21:15:02Z

lib/src/model.dart

@@ -2024,6 +2255,10 @@ abstract class ModelElement implements Comparable, Nameable, Documentable {
    return _canonicalLibrary;
  }

+  List<ScoredCandidate> scoreCanonicalCandidates(List<Library> libraries) {
+    return libraries.map((l) => scoreElementWithLibrary(l)).toList()..sort();


Ditto here.

But of course, feel free to ignore!

This one, since it includes a map in the middle and includes function parameters seems more natural with the long form.

pq · 2017-06-05T21:17:12Z

lib/src/model.dart

+
+  PackageWarningHelpText(this.warning, this.warningName, this.shortHelp,
+      [this.longHelp]) {
+    if (this.longHelp == null) this.longHelp = [];


I think you could do this.longHelp ??= []...

pq · 2017-06-05T21:19:47Z

lib/src/model.dart

-    _countedWarnings.putIfAbsent(element, () => new Set());
-    _countedWarnings[element].add(warningData);
-    _writeWarning(kind, fullMessage);
+    _countedWarnings.putIfAbsent(element?.element, () => new Set());


It looks like you're allowing for mappings with null keys. Is that right?

Right. Null keys are allowed when we have warnings that aren't related to a specific element.

pq · 2017-06-05T21:28:23Z

test/model_test.dart

+
+    test('@canonicalFor directive works', () {
+      expect(SomeOtherClass.canonicalLibrary, equals(reexportOneLib));
+      expect(SomeClass.canonicalLibrary, equals(reexportTwoLib));


I think you can elide the equals?

Ah, apparently so. Done.

…zation-scorer

jcollins-g · 2017-06-13T18:15:40Z

Realizing I didn't reply here to @devoncarew about reducing SDK errors. We talked about this briefly offline, but for the record:

This is only for ambiguous reexport warning. When determining canonicalization for a given symbol, there are three possible paths:

There are multiple canonical locations and we have to choose between them.
There is exactly one possible canonical location.
There is no canonicalization available.

Earlier work in #1390 and #1368 implemented the second item above, and partially implemented the first in a way that worked well for Flutter but not AngularDart. This PR improves the first case to the maximum extent I can think of for now and gives control to users for disambiguating cases it can't handle. Improving the third case (no canonicalization found) is still in the queue and will require knowledge of packages not being documented.

devoncarew · 2017-06-13T19:11:17Z

My comments mostly just applied to the UI of the tool; lgtm for my part

jcollins-g added 14 commits April 13, 2017 14:48

First stab at disambiguation, but needs to be extracted

c213186

update from head

1dab2d0

scorer in pretty good shape now

787d4f8

Merge branch 'master' of github.com:dart-lang/dartdoc into canonicali…

bddeab3

…zation-scorer

beginnings of being able to set canonicalization manually

13717b5

Merge branch 'master' of github.com:dart-lang/dartdoc into canonicali…

004265c

…zation-scorer

intermediate state: moving to library-based canonicalFor declaration

2ef88ba

Merge branch 'master' of github.com:dart-lang/dartdoc into canonicali…

6258331

…zation-scorer

intermediate state -- autoinclude deps broken

5fa4ca1

intermediate: tests written and working

4dedc21

test really works now.

7d307a9

regen test docs

899eb15

Add new test doc directories

7fa1209

dartfmt

24e883f

jcollins-g requested a review from keertip June 2, 2017 22:30

googlebot added the cla: yes Google CLA check succeeded. label Jun 2, 2017

Comment typo.

c3de1bd

devoncarew reviewed Jun 5, 2017

View reviewed changes

jcollins-g added 3 commits June 5, 2017 08:43

Fix --auto-include-deps

d997568

cleanup

7ed0790

rebuild docs

5bfff11

Helper moved with the fix.

abf875f

Hide the debug option for the scorer confidence

e16af38

jcollins-g requested a review from pq June 5, 2017 18:52

Remove unnecessary casts

8e880c4

pq reviewed Jun 5, 2017

View reviewed changes

pq approved these changes Jun 5, 2017

View reviewed changes

pq reviewed Jun 5, 2017

View reviewed changes

Implement review comments and extract namePieces into Nameable

a69203a

jcollins-g removed the request for review from keertip June 7, 2017 17:31

jcollins-g added 2 commits June 13, 2017 10:51

Merge branch 'master' of github.com:dart-lang/dartdoc into canonicali…

9fe85f8

…zation-scorer

Regenerate docs

bc47ab2

jcollins-g merged commit 4a505cf into master Jun 13, 2017

jcollins-g deleted the canonicalization-scorer branch June 13, 2017 19:29

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Canonicalization scorer #1455

Canonicalization scorer #1455

jcollins-g commented Jun 2, 2017 •

edited

Loading

kwalrath commented Jun 2, 2017

Hixie commented Jun 2, 2017

chalin commented Jun 3, 2017

devoncarew left a comment •

edited

Loading

devoncarew Jun 5, 2017

devoncarew Jun 5, 2017

jcollins-g Jun 5, 2017

devoncarew Jun 5, 2017

jcollins-g Jun 5, 2017

jcollins-g commented Jun 5, 2017

jcollins-g commented Jun 5, 2017 •

edited

Loading

pq Jun 5, 2017

jcollins-g Jun 6, 2017

pq Jun 5, 2017

jcollins-g Jun 6, 2017

pq left a comment

pq Jun 5, 2017

jcollins-g Jun 6, 2017

pq Jun 5, 2017

pq Jun 5, 2017

jcollins-g Jun 6, 2017

pq Jun 5, 2017

jcollins-g Jun 6, 2017

pq Jun 5, 2017

jcollins-g Jun 6, 2017

pq Jun 5, 2017

jcollins-g Jun 6, 2017

jcollins-g commented Jun 13, 2017

devoncarew commented Jun 13, 2017

Canonicalization scorer #1455

Canonicalization scorer #1455

Conversation

jcollins-g commented Jun 2, 2017 • edited Loading

kwalrath commented Jun 2, 2017

Hixie commented Jun 2, 2017

chalin commented Jun 3, 2017

devoncarew left a comment • 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

jcollins-g commented Jun 5, 2017

jcollins-g commented Jun 5, 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

pq left a 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 Jun 13, 2017

devoncarew commented Jun 13, 2017

jcollins-g commented Jun 2, 2017 •

edited

Loading

devoncarew left a comment •

edited

Loading

jcollins-g commented Jun 5, 2017 •

edited

Loading