diff --git a/lib/src/model.dart b/lib/src/model.dart index effeeff6fd..530a9d6b31 100644 --- a/lib/src/model.dart +++ b/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,74 +3558,101 @@ 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 uniqueNames = new Set(); + final RegExp validIdRegExp = new RegExp(r'^[a-zA-Z_]\w*$'); + + final Set uniqueIds = new Set(); + 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 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 @@ -3629,12 +3660,12 @@ abstract class ModelElement extends Canonicalization return '''
-
- @@ -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 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 args = matches.map((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 _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 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 args = { + '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 messageParts = [warningMessage]; diff --git a/lib/src/warnings.dart b/lib/src/warnings.dart index 5135c915f5..e7e182a10e 100644 --- a/lib/src/warnings.dart +++ b/lib/src/warnings.dart @@ -90,6 +90,10 @@ final Map 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. diff --git a/test/model_test.dart b/test/model_test.dart index eb638a7613..bd3077ec76 100644 --- a/test/model_test.dart +++ b/test/model_test.dart @@ -510,19 +510,36 @@ void main() { group('Animation', () { Class dog; Method withAnimation; + Method withNamedAnimation; + Method withQuoteNamedAnimation; + Method withInvalidNamedAnimation; + Method withDeprecatedAnimation; Method withAnimationNonUnique; + Method withAnimationNonUniqueDeprecated; Method withAnimationWrongParams; Method withAnimationBadWidth; Method withAnimationBadHeight; Method withAnimationInOneLineDoc; Method withAnimationInline; + Method withAnimationOutOfOrder; + Method withAnimationUnknownArg; setUp(() { dog = exLibrary.classes.firstWhere((c) => c.name == 'Dog'); withAnimation = dog.allInstanceMethods.firstWhere((m) => m.name == 'withAnimation'); + withNamedAnimation = dog.allInstanceMethods + .firstWhere((m) => m.name == 'withNamedAnimation'); + withQuoteNamedAnimation = dog.allInstanceMethods + .firstWhere((m) => m.name == 'withQuotedNamedAnimation'); + withInvalidNamedAnimation = dog.allInstanceMethods + .firstWhere((m) => m.name == 'withInvalidNamedAnimation'); + withDeprecatedAnimation = dog.allInstanceMethods + .firstWhere((m) => m.name == 'withDeprecatedAnimation'); withAnimationNonUnique = dog.allInstanceMethods .firstWhere((m) => m.name == 'withAnimationNonUnique'); + withAnimationNonUniqueDeprecated = dog.allInstanceMethods + .firstWhere((m) => m.name == 'withAnimationNonUniqueDeprecated'); withAnimationWrongParams = dog.allInstanceMethods .firstWhere((m) => m.name == 'withAnimationWrongParams'); withAnimationBadWidth = dog.allInstanceMethods @@ -533,20 +550,70 @@ void main() { .firstWhere((m) => m.name == 'withAnimationInOneLineDoc'); withAnimationInline = dog.allInstanceMethods .firstWhere((m) => m.name == 'withAnimationInline'); + withAnimationOutOfOrder = dog.allInstanceMethods + .firstWhere((m) => m.name == 'withAnimationOutOfOrder'); + withAnimationUnknownArg = dog.allInstanceMethods + .firstWhere((m) => m.name == 'withAnimationUnknownArg'); packageGraph.allLocalModelElements.forEach((m) => m.documentation); }); - test("renders an animation within the method documentation", () { + test("renders an unnamed animation within the method documentation", () { + expect(withAnimation.documentation, contains('