External tool support #1756

gspencergoog · 2018-09-18T06:54:00Z

This adds the ability to invoke external tools on the text between the {@tool ...} and {@end-tool} directives. The available tools are defined in the dartdoc_options.yaml file, under a tools: section that maps tool names to executable locations. If a tool executable ends in ".dart", then the tool will be invoked with the dart executable automatically.

For example, if the "sort" tool was mapped to "/bin/sort" (on Unix), then the following input:

/// ###Fruit
/// {@tool sort -u $INPUT}
///  - banana
///  - apple
///  - cucumber
///  - banana
/// {@end-tool}

would result in output something like:

Fruit

apple
banana
cucumber

jcollins-g · 2018-09-18T16:53:32Z

Nice to see this coming together. I have some initial thoughts at a high level, happy to share if you're ready for that.

gspencergoog · 2018-09-18T16:58:08Z

Sure, high level thoughts would be great!

jcollins-g · 2018-09-18T18:08:13Z

This is all pretty half baked and I only glanced through what you have, so you may have implemented some of this and I didn't see it.

Interested to see how well multi-platform works. It appears to at least work a little bit :-)
Implementing restrictions on tools for dartdoc and having a reasonable fallback behavior / warning when tools are disabled seems like a good idea. It might even be an optional part of the tool directive -- what docs to display if we can't use the tool.
Some sort of guidelines on how to use this?
Can tool directives output documentation that contains other directives, like animation or even nodoc?
Comment references are mostly ignored from the analyzer but not entirely. If we support adding comment references as part of a tool directive, tests for that condition may tease out some complicated stuff in the markdown extension we use for resolution.

gspencergoog · 2018-09-18T19:31:14Z

Multiplatform works OK if you implement your external tool in Dart, but for instance running "/bin/sh" won't work on Windows, of course. Do you have any suggestions for how the platform configuration should look? I was thinking something like this, maybe:

dartdoc:
  tools:
    drill:
      command: ['bin/drill.dart']
      description: 'Makes a hole'
    hammer:
      macos: ['/bin/sh', '-c', 'echo']
      linux: ['/bin/sh', '-c', 'echo']
      windows: ['C:\\Windows\\System32\\cmd.exe', '/c', 'echo']
      description: 'Works on everything'

Creating that with the dartdoc options structure seems complicated though: I'm not sure how well it handles things that have different types (e.g. a map with lists of strings OR strings as values).

What kind of "restrictions" do you mean? Platform restrictions? Or do you mean when the tool isn't found in the tool map?
Docs are coming (those are the docs I mentioned in the PR description)
Tool directives can emit anything, including other directives. Tool directives are evaluated first for this reason. I was even thinking of making a new directive that they could emit that would embed HTML verbatim instead of markdown.
What do you mean by "comment references"? The [MyClass] references? In any case, having the markdown come from a tool or being embedded directly should make no difference to the markdown extension, except that I doubt the analyzer will ever be able to evaluate dartdoc directives, right?

jcollins-g · 2018-09-18T19:46:31Z

Multiplatform and complicated structures: I just added the ability to convert YamlMaps to arbitrary structures in DartdocOptions, and that may make it easier. See the implementation of the categories option from #1754. I don't have a strong opinion on the structure, other than what I see in your example seems close but not quite. I'll think about it.

Restrictions: Yeah, I wasn't clear. I meant the ability to have tools disabled entirely, and to restrict tools to a subdirectory of the package.

Comment references: I am not certain but I think we may still do some checks around the analyzer data for comment references, and use them as a last resort for resolution. Adding tests particularly around the case where a tool directive changes the relative offset of an existing comment reference, adding a comment reference as part of a tool directive, and adding a comment reference as part of a tool directive when the comment otherwise has no references would be good tests.

gspencergoog · 2018-09-18T20:28:52Z

A "disabled" tool just doesn't appear in the map. We could have a disabled bool in the map, I guess, but that's just like commenting it out in the options file. Any tool that is referenced, but not available in the configuration will cause dartdoc to error out in the current design.

I hadn't thought about restricting tools to a subdir: do you mean that the executable can only appear in a subdir? Or that it is basically chrooted to a subdir? I think that since the map is explicit about the executable, that restricting it to come from a subdir is redundant. chroot-style restrictions won't work on Windows, and I don't know any other good way to restrict access to a process.

Comment references: So you're worried about changing the offsets to references? Tools doesn't do anything different from the {@example} directive, really, so as long as that doesn't mess up references, this shouldn't either. I'll look at adding the tests, though, those seem like a good idea.

jcollins-g · 2018-09-18T20:33:47Z

I was more talking about a way to disable tools from the command line. But maybe it's not as important as I thought given the other considerations you mentioned. As for restricting which tools get run, I suppose it is rather redundant. Restricting tool access to a subdirectory doesn't stop that tool from forking off something arbitrary either.

Good point on examples. The main thing I am curious about then is adding (and removing?) comment references.

gspencergoog · 2018-09-18T22:06:18Z

@jcollins-g, How do I update the docs that failed to compare in this build? I've run grind update-test-package-docs already.

jcollins-g · 2018-09-18T22:08:07Z

@gspencergoog There's currently no way to do that without using the stable version of the SDK. I use a script to switch versions so I can do it quickly.

Basically, switch to dart 2, run pub get and then pub run grinder update-test-package-docs, and that will update the right version.

gspencergoog · 2018-09-20T19:25:44Z

OK, I think this is ready for more detailed review. I've modified the options structure to be what I outlined above: thanks for that Categories work, @jcollins-g that really made it work well!

I tried to think of better ways to organize it, and I think it works as-is. I did include a "description" field, although I don't really know what to use that for yet. Maybe it should just be removed.

jcollins-g · 2018-09-21T16:28:25Z

I was out sick two days, but I will try to get to this review today. Sorry for the delay.

gspencergoog · 2018-09-21T16:53:06Z

No worries. I just realized that I haven't gone back through and removed unnecessary type annotations, as is the dartdoc style. No need to bother commenting on those, I'll do that.

jcollins-g

I think adding a the tests I mentioned would be great.

Also, a blurb about the tool directive in README.md seems like a good idea to me.

jcollins-g · 2018-09-24T16:25:42Z

lib/src/dartdoc_options.dart

@@ -110,6 +110,103 @@ class CategoryConfiguration {
  }
 }

+class ToolDefinition {


Would be nice if there were some brief docs for the new classes, at least

jcollins-g · 2018-09-24T16:27:03Z

lib/src/dartdoc_options.dart

-          .map((String mapKey, String mapValue) => new MapEntry<String, String>(
-              mapKey, pathContext.canonicalize(resolveTildePath(mapValue))))
-          .cast<String, String>() as T;
+          .map<String, String>((String key, String value) {


ah yes, better

jcollins-g · 2018-09-24T17:33:52Z

test/model_test.dart

+    test("can invoke a tool", () {
+      expect(invokeTool.documentation, contains('## `Yes it is!`'));
+    });
+    test(r"can invoke a tool with no $INPUT or args", () {


Would like to see tests of cases where a comment reference is added/removed.

OK, I added that, but I'm not sure how to verify that the link was actually recognized and linked.

Ah. You can use the member documentationAsHtml to do that.

dartdoc/lib/src/model.dart

Line 1163 in 251962a

String get documentationAsHtml;

There are some circumstances where this might not work at head; if that check doesn't work put a skip on the test and mention PR #1765.

gspencergoog · 2018-09-28T20:03:39Z

OK, I've updated the README too. PTAL.

jcollins-g

LGTM after last test tweak

jcollins-g · 2018-09-28T20:15:38Z

test/model_test.dart

+    test("can invoke a tool", () {
+      expect(invokeTool.documentation, contains('## `Yes it is!`'));
+    });
+    test(r"can invoke a tool with no $INPUT or args", () {


Ah. You can use the member documentationAsHtml to do that.

dartdoc/lib/src/model.dart

Line 1163 in 251962a

String get documentationAsHtml;

There are some circumstances where this might not work at head; if that check doesn't work put a skip on the test and mention PR #1765.

gspencergoog · 2018-10-02T18:28:31Z

OK, last test tweak is in. @jcollins-g Feel free to commit this.

googlebot added the cla: yes Google CLA check succeeded. label Sep 18, 2018

gspencergoog force-pushed the tools branch 2 times, most recently from ae32955 to f3bfb4c Compare September 18, 2018 16:27

gspencergoog force-pushed the tools branch 2 times, most recently from a7a01a9 to 5c970c9 Compare September 18, 2018 21:40

gspencergoog force-pushed the tools branch 5 times, most recently from 88283d1 to 3a5b586 Compare September 20, 2018 15:40

gspencergoog changed the title ~~[WIP] External tool support~~ External tool support Sep 20, 2018

gspencergoog force-pushed the tools branch from 61bf4c9 to 584c1b1 Compare September 21, 2018 16:44

jcollins-g reviewed Sep 24, 2018

View reviewed changes

gspencergoog force-pushed the tools branch from f8eda02 to af62757 Compare September 28, 2018 17:56

jcollins-g approved these changes Sep 28, 2018

View reviewed changes

gspencergoog added 2 commits October 2, 2018 11:19

Add tool support

538301f

Updating Docs

74731d5

gspencergoog added 11 commits October 2, 2018 11:19

Updating CONTRIBUTING.md

1f2d6e6

Adding more complete tool options.

8bf0ed5

Update docs on Linux again.

f370499

Add tool support

a0b4a40

Updating Docs

1474ef4

Updating CONTRIBUTING.md

2bfcff6

Adding more complete tool options.

a5c8ea5

Update docs on Linux again.

4270421

Review changes and associated docs updates

10e0b43

Update README.md

e656a5c

Remove unnecessary type declarations.

b5d60c6

gspencergoog force-pushed the tools branch from 93f33db to b5d60c6 Compare October 2, 2018 18:20

Review Changes

b729945

jcollins-g merged commit 3494543 into dart-lang:master Oct 2, 2018

External tool support #1756

External tool support #1756

Uh oh!

Conversation

gspencergoog commented Sep 18, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Fruit

Uh oh!

jcollins-g commented Sep 18, 2018

Uh oh!

gspencergoog commented Sep 18, 2018

Uh oh!

jcollins-g commented Sep 18, 2018

Uh oh!

gspencergoog commented Sep 18, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

jcollins-g commented Sep 18, 2018

Uh oh!

gspencergoog commented Sep 18, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

jcollins-g commented Sep 18, 2018

Uh oh!

gspencergoog commented Sep 18, 2018

Uh oh!

jcollins-g commented Sep 18, 2018

Uh oh!

gspencergoog commented Sep 20, 2018

Uh oh!

jcollins-g commented Sep 21, 2018

Uh oh!

gspencergoog commented Sep 21, 2018

Uh oh!

jcollins-g left a comment

Choose a reason for hiding this comment

Uh oh!

jcollins-g Sep 24, 2018

Choose a reason for hiding this comment

Uh oh!

gspencergoog Sep 28, 2018

Choose a reason for hiding this comment

Uh oh!

jcollins-g Sep 24, 2018

Choose a reason for hiding this comment

Uh oh!

jcollins-g Sep 24, 2018

Choose a reason for hiding this comment

Uh oh!

gspencergoog Sep 28, 2018

Choose a reason for hiding this comment

Uh oh!

jcollins-g Sep 28, 2018

Choose a reason for hiding this comment

Uh oh!

gspencergoog commented Sep 28, 2018

Uh oh!

jcollins-g left a comment

Choose a reason for hiding this comment

Uh oh!

jcollins-g Sep 28, 2018

Choose a reason for hiding this comment

Uh oh!

gspencergoog commented Oct 2, 2018

Uh oh!

Uh oh!

gspencergoog commented Sep 18, 2018 •

edited

Loading

gspencergoog commented Sep 18, 2018 •

edited

Loading

gspencergoog commented Sep 18, 2018 •

edited

Loading