Remove name parameter from animation directive. (#1715)

Author: gspencergoog

lib/src/model.dart

@@ -34,6 +34,7 @@ import 'package:analyzer/src/generated/source_io.dart';
 import 'package:analyzer/src/dart/element/member.dart'
     show ExecutableMember, Member, ParameterMember;
 import 'package:analyzer/src/dart/analysis/driver.dart';
+import 'package:args/args.dart';
 import 'package:collection/collection.dart';
 import 'package:dartdoc/src/dartdoc_options.dart';
 import 'package:dartdoc/src/element_type.dart';
@@ -3487,12 +3488,9 @@ abstract class ModelElement extends Canonicalization
   ///
   ///     {@example PATH [region=NAME] [lang=NAME]}
   ///
-  /// where PATH and NAME are tokens _without_ whitespace; NAME can optionally be
-  /// quoted (use of quotes is for backwards compatibility and discouraged).
-  ///
   /// If PATH is `dir/file.ext` and region is `r` then we'll look for the file
-  /// named `dir/file-r.ext.md`, relative to the project root directory (of the
-  /// project for which the docs are being generated).
+  /// named `dir/file-r.ext.md`, relative to the project root directory of the
+  /// project for which the docs are being generated.
   ///
   /// Examples: (escaped in this comment to show literal values in dartdoc's
   ///            dartdoc)
@@ -3505,6 +3503,10 @@ abstract class ModelElement extends Canonicalization
     RegExp exampleRE = new RegExp(r'{@example\s+([^}]+)}');
     return rawdocs.replaceAllMapped(exampleRE, (match) {
       var args = _getExampleArgs(match[1]);
+      if (args == null) {
+        // Already warned about an invalid parameter if this happens.
+        return '';
+      }
       var lang =
           args['lang'] ?? pathLib.extension(args['src']).replaceFirst('.', '');
 
@@ -3533,17 +3535,19 @@ abstract class ModelElement extends Canonicalization
   ///
   /// Syntax:
   ///
-  ///     {@animation NAME WIDTH HEIGHT URL}
+  ///     {@animation WIDTH HEIGHT URL [id=ID]}
   ///
   /// Example:
   ///
-  ///     {@animation my_video 300 300 https://example.com/path/to/video.mp4}
+  ///     {@animation 300 300 https://example.com/path/to/video.mp4 id="my_video"}
   ///
   /// Which will render the HTML necessary for embedding a simple click-to-play
-  /// HTML5 video player with no controls.
+  /// HTML5 video player with no controls that has an HTML id of "my_video".
   ///
-  /// The NAME should be a unique name that is a valid javascript identifier,
-  /// and will be used as the id for the video tag.
+  /// The optional ID should be a unique id that is a valid JavaScript
+  /// identifier, and will be used as the id for the video tag. If no ID is
+  /// supplied, then a unique identifier (starting with "animation_") will be
+  /// generated.
   ///
   /// The width and height must be integers specifying the dimensions of the
   /// video file in pixels.
@@ -3554,87 +3558,114 @@ abstract class ModelElement extends Canonicalization
     final RegExp basicAnimationRegExp =
         new RegExp(r'''{@animation\s+([^}]+)}''');
 
-    // Animations have four parameters, and the last one can be surrounded by
-    // quotes (which are ignored). This RegExp is used to validate the directive
-    // for the correct number of parameters.
-    final RegExp animationRegExp =
-        new RegExp(r'''{@animation\s+([^}\s]+)\s+([^}\s]+)\s+([^}\s]+)'''
-            r'''\s+['"]?([^}]+)['"]?}''');
-
     // Matches valid javascript identifiers.
-    final RegExp validNameRegExp = new RegExp(r'^[a-zA-Z_][a-zA-Z0-9_]*$');
-
-    // Keeps names unique.
-    final Set<String> uniqueNames = new Set<String>();
+    final RegExp validIdRegExp = new RegExp(r'^[a-zA-Z_]\w*$');
+
+    final Set<String> uniqueIds = new Set<String>();
+    String getUniqueId(String base) {
+      int count = 1;
+      String id = '$base$count';
+      while (uniqueIds.contains(id)) {
+        count++;
+        id = '$base$count';
+      }
+      return id;
+    }
 
     return rawDocs.replaceAllMapped(basicAnimationRegExp, (basicMatch) {
-      final Match match = animationRegExp.firstMatch(basicMatch[0]);
-      if (match == null) {
+      final ArgParser parser = new ArgParser();
+      parser.addOption('id');
+      final ArgResults args = _parseArgs(basicMatch[1], parser, 'animation');
+      if (args == null) {
+        // Already warned about an invalid parameter if this happens.
+        return '';
+      }
+      final List<String> positionalArgs = args.rest.sublist(0);
+      String uniqueId;
+      bool wasDeprecated = false;
+      if (positionalArgs.length == 4) {
+        // Supports the original form of the animation tag for backward
+        // compatibility.
+        uniqueId = positionalArgs.removeAt(0);
+        wasDeprecated = true;
+      } else if (positionalArgs.length == 3) {
+        uniqueId = args['id'] ?? getUniqueId('animation_');
+      } else {
         warn(PackageWarning.invalidParameter,
-            message: 'Invalid @animation directive: ${basicMatch[0]}\n'
-                'Animation directives must be of the form: {@animation NAME '
-                'WIDTH HEIGHT URL}');
+            message: 'Invalid @animation directive, "${basicMatch[0]}"\n'
+                'Animation directives must be of the form "{@animation WIDTH '
+                'HEIGHT URL [id=ID]}"');
         return '';
       }
-      String name = match[1];
-      if (!validNameRegExp.hasMatch(name)) {
+
+      if (!validIdRegExp.hasMatch(uniqueId)) {
         warn(PackageWarning.invalidParameter,
-            message: 'An animation has an invalid name: $name. The name can '
-                'only contain letters, numbers and underscores.');
+            message: 'An animation has an invalid identifier, "$uniqueId". The '
+                'identifier can only contain letters, numbers and underscores, '
+                'and must not begin with a number.');
+        return '';
+      }
+      if (uniqueIds.contains(uniqueId)) {
+        warn(PackageWarning.invalidParameter,
+            message: 'An animation has a non-unique identifier, "$uniqueId". '
+                'Animation identifiers must be unique.');
         return '';
-      } else {
-        if (uniqueNames.contains(name)) {
-          warn(PackageWarning.invalidParameter,
-              message:
-                  'An animation has a non-unique name: $name. Animation names '
-                  'must be unique.');
-          return '';
-        }
-        uniqueNames.add(name);
       }
+      uniqueIds.add(uniqueId);
+
       int width;
       try {
-        width = int.parse(match[2]);
+        width = int.parse(positionalArgs[0]);
       } on FormatException {
         warn(PackageWarning.invalidParameter,
-            message:
-                'An animation has an invalid width ($name): ${match[2]}. The '
-                'width must be an integer.');
+            message: 'An animation has an invalid width ($uniqueId), '
+                '"${positionalArgs[0]}". The width must be an integer.');
         return '';
       }
+
       int height;
       try {
-        height = int.parse(match[3]);
+        height = int.parse(positionalArgs[1]);
       } on FormatException {
         warn(PackageWarning.invalidParameter,
-            message:
-                'An animation has an invalid height ($name): ${match[3]}. The '
-                'height must be an integer.');
+            message: 'An animation has an invalid height ($uniqueId), '
+                '"${positionalArgs[1]}". The height must be an integer.');
         return '';
       }
+
       Uri movieUrl;
       try {
-        movieUrl = Uri.parse(match[4]);
+        movieUrl = Uri.parse(positionalArgs[2]);
       } on FormatException catch (e) {
         warn(PackageWarning.invalidParameter,
-            message:
-                'An animation URL could not be parsed ($name): ${match[4]}\n$e');
+            message: 'An animation URL could not be parsed ($uniqueId): '
+                '${positionalArgs[2]}\n$e');
         return '';
       }
-      final String overlayName = '${name}_play_button_';
+      final String overlayId = '${uniqueId}_play_button_';
+
+      // Only warn about deprecation if some other warning didn't occur.
+      if (wasDeprecated) {
+        warn(PackageWarning.deprecated,
+            message:
+                'Deprecated form of @animation directive, "${basicMatch[0]}"\n'
+                'Animation directives are now of the form "{@animation '
+                'WIDTH HEIGHT URL [id=ID]}" (id is an optional '
+                'parameter)');
+      }
 
       // Blank lines before and after, and no indenting at the beginning and end
       // is needed so that Markdown doesn't confuse this with code, so be
       // careful of whitespace here.
       return '''
 
 <div style="position: relative;">
-  <div id="${overlayName}"
-       onclick="if ($name.paused) {
-                  $name.play();
+  <div id="${overlayId}"
+       onclick="if ($uniqueId.paused) {
+                  $uniqueId.play();
                   this.style.display = 'none';
                 } else {
-                  $name.pause();
+                  $uniqueId.pause();
                   this.style.display = 'block';
                 }"
        style="position:absolute;
@@ -3645,14 +3676,14 @@ abstract class ModelElement extends Canonicalization
               background-repeat: no-repeat;
               background-image: url(static-assets/play_button.svg);">
   </div>
-  <video id="$name"
+  <video id="$uniqueId"
          style="width:${width}px; height:${height}px;"
          onclick="if (this.paused) {
                     this.play();
-                    $overlayName.style.display = 'none';
+                    $overlayId.style.display = 'none';
                   } else {
                     this.pause();
-                    $overlayName.style.display = 'block';
+                    $overlayId.style.display = 'block';
                   }" loop>
     <source src="$movieUrl" type="video/mp4"/>
   </video>
@@ -3718,29 +3749,80 @@ abstract class ModelElement extends Canonicalization
     });
   }
 
+  /// Helper to process arguments given as a (possibly quoted) string.
+  ///
+  /// First, this will split the given [argsAsString] into separate arguments,
+  /// taking any quoting (either ' or " are accepted) into account, including
+  /// handling backslash-escaped quotes.
+  ///
+  /// Then, it will prepend "--" to any args that start with an identifier
+  /// followed by an equals sign, allowing the argument parser to treat any
+  /// "foo=bar" argument as "--foo=bar". It does handle quoted args like
+  /// "foo='bar baz'" too, returning just bar (without quotes) for the foo
+  /// value.
+  ///
+  /// It then parses the resulting argument list normally with [argParser] and
+  /// returns the result.
+  ArgResults _parseArgs(
+      String argsAsString, ArgParser argParser, String directiveName) {
+    // Regexp to take care of splitting arguments, and handling the quotes
+    // around arguments, if any.
+    //
+    // Match group 1 is the "foo=" part of the option, if any.
+    // Match group 2 contains the quote character used (which is discarded).
+    // Match group 3 is a quoted arg, if any, without the quotes.
+    // Match group 4 is the unquoted arg, if any.
+    final RegExp argMatcher = new RegExp(r'([a-zA-Z\-_0-9]+=)?' // option name
+        r'(?:' // Start a new non-capture group for the two possibilities.
+        r'''(["'])((?:\\{2})*|(?:.*?[^\\](?:\\{2})*))\2|''' // with quotes.
+        r'([^ ]+))'); // without quotes.
+    final Iterable<Match> matches = argMatcher.allMatches(argsAsString);
+
+    // Remove quotes around args, and for any args that look like assignments
+    // (start with valid option names followed by an equals sign), add a "--" in front
+    // so that they parse as options.
+    final Iterable<String> args = matches.map<String>((Match match) {
+      String option = '';
+      if (match[1] != null) {
+        option = '--${match[1]}';
+      }
+      return option + (match[3] ?? '') + (match[4] ?? '');
+    });
+
+    try {
+      return argParser.parse(args);
+    } on ArgParserException catch (e) {
+      warn(PackageWarning.invalidParameter,
+          message: 'The {@$directiveName ...} directive was called with '
+              'invalid parameters. $e');
+      return null;
+    }
+  }
+
   /// Helper for _injectExamples used to process @example arguments.
   /// Returns a map of arguments. The first unnamed argument will have key 'src'.
   /// The computed file path, constructed from 'src' and 'region' will have key
   /// 'file'.
   Map<String, String> _getExampleArgs(String argsAsString) {
-    // Extract PATH and return is under key 'src'
-    var endOfSrc = argsAsString.indexOf(' ');
-    if (endOfSrc < 0) endOfSrc = argsAsString.length;
-    var src = argsAsString.substring(0, endOfSrc);
-    src = src.replaceAll('/', Platform.pathSeparator);
-    final args = {'src': src};
-
-    // Process remaining named arguments
-    var namedArgs = argsAsString.substring(endOfSrc);
-    // Arg value: allow optional quotes; warning: we still don't support whitespace.
-    RegExp keyValueRE = new RegExp('(\\w+)=[\'"]?(\\S*)[\'"]?');
-    Iterable<Match> matches = keyValueRE.allMatches(namedArgs);
-    matches.forEach((match) {
-      // Ignore optional quotes
-      args[match[1]] = match[2].replaceAll(new RegExp('[\'"]'), '');
-    });
+    ArgParser parser = new ArgParser();
+    parser.addOption('lang');
+    parser.addOption('region');
+    ArgResults results = _parseArgs(argsAsString, parser, 'example');
+    if (results == null) {
+      return null;
+    }
 
-    // Compute 'file'
+    // Extract PATH and fix the path separators.
+    final String src = results.rest.isEmpty
+        ? ''
+        : results.rest.first.replaceAll('/', Platform.pathSeparator);
+    final Map<String, String> args = <String, String>{
+      'src': src,
+      'lang': results['lang'],
+      'region': results['region'] ?? '',
+    };
+
+    // Compute 'file' from region and src.
     final fragExtension = '.md';
     var file = src + fragExtension;
     var region = args['region'] ?? '';
@@ -4233,6 +4315,9 @@ class PackageGraph {
       case PackageWarning.invalidParameter:
         warningMessage = 'invalid parameter to dartdoc directive: ${message}';
         break;
+      case PackageWarning.deprecated:
+        warningMessage = 'deprecated dartdoc usage: ${message}';
+        break;
     }
 
     List<String> messageParts = [warningMessage];

lib/src/warnings.dart

@@ -90,6 +90,10 @@ final Map<PackageWarning, PackageWarningHelpText> packageWarningText = const {
       PackageWarning.invalidParameter,
       "invalidParameter",
       "A parameter given to a dartdoc directive was invalid."),
+  PackageWarning.deprecated: const PackageWarningHelpText(
+      PackageWarning.deprecated,
+      "deprecated",
+      "A dartdoc directive has a deprecated format."),
 };
 
 /// Something that package warnings can be called on.  Optionally associated
@@ -130,6 +134,7 @@ enum PackageWarning {
   missingFromSearchIndex,
   typeAsHtml,
   invalidParameter,
+  deprecated,
 }
 
 /// Warnings it is OK to skip if we can determine the warnable isn't documented.