Remove name parameter from animation directive. #1715
Conversation
lib/src/model.dart
Outdated
| return option + (match[3] ?? '') + (match[4] ?? ''); | ||
| }); | ||
|
|
||
| return argParser.parse(args); |
There was a problem hiding this comment.
I think ArgParserExceptions thrown here probably should not be fatal and should instead emit some sort of helpful warning. You might have to carry along some extra data into _parseArgs to make that useful without reflection.
There was a problem hiding this comment.
Good point . Would it be enough to just call warn with invalidParameter here (which is an error) and return an empty ArgResults in the case of an ArgParserException?
| if (positionalArgs.length == 4) { | ||
| // Supports the original form of the animation tag for backward | ||
| // compatibility. | ||
| uniqueId = positionalArgs.removeAt(0); |
There was a problem hiding this comment.
This will of course completely discard the name. Given that anchors do not actually work yet I suppose this is probably harmless?
There was a problem hiding this comment.
No, this is just is converting the old form of positional args (four args) to the new form (three args) by removing the first arg and using it as the uniqueName. The id is actually used in the injected HTML to control the playing of the video, so the uniqueness is required, as is the presence of some sort of id.
|
|
||
| /// Animation method with invalid name | ||
| /// | ||
| /// {@animation 100 100 http://host/path/to/video.mp4 id=2isNot-A-ValidName} |
There was a problem hiding this comment.
Suggest adding test cases for {@animation 250 250 id=outOfOrder http://host/path/to/video.mp4}
and {@animation 149 150 http://host/path/to/video.mp4 notId=unexpected} to validate more edge case behavior
gspencergoog
force-pushed
the
remove_name
branch
3 times, most recently
from
17be545 to
ea80a75
Compare
gspencergoog
force-pushed
the
remove_name
branch
from
ea80a75 to
f232b65
Compare
| if (positionalArgs.length == 4) { | ||
| // Supports the original form of the animation tag for backward | ||
| // compatibility. | ||
| uniqueId = positionalArgs.removeAt(0); |
|
I think github's commenting system doesn't like it when commits are replaced on the branch rather than added as the review takes place. github doesn't seem to want to offer anything except an emoji response to your reply on ArgParserException and throws some errors because it can't find the previous commit that was attached to. |
This PR removes the required first argument for the
{@animation...}directive, making it into an optional argument.It also builds a way to parse arguments for directives that is more consistent, and leverages the Dart arg parser. The arguments may be quoted in order to contain spaces if needed. Updates _injectExamples to use this parser as well as the _injectAnimations function.
The old animation directive form is still supported, but will give a deprecation warning if used. I also added a new type of warning for this:
PackageWarning.deprecated.