Comments break video plugin

[opn]

The recent commit has broken the plugin for a large range of comments. I've tracked it down with a few examples. See http://issues.fedwiki.org/video-plugin-issue.html

[paul90]

Thanks for raising this issue.

It looks to be caused by a comment, of more than one word, starting with an all uppercase word and effectively being treated as if it part of the DSL rather than the comment.

[WardCunningham]

When I look at the logic in the parse function I don't see the lists of keywords I would expect. Rather I see code trying to infer meaning from a scramble of nearly identical regex metacharacters. I suspect I was ultimately at fault. Maybe I was trying to make YOUTUBE and VIMEO both work in a single case but such economy has lead us astray. Time to refactor?

My first thought is to write a little tool that will first find all of the Video plugins in the federation so I have an exhaustive set of test cases before slinging massive changes in a parse refactoring.

[WardCunningham]

Hmm. Just pulled down the plugin. Npm installed. Grunt says:

I'm going to wait on debugging this because I am all in on building a database of Video plugin markup from the federation.

[dobbs]

First, the "failed to parse URL" message is definitely related to the code I added to support HTML5 videos.

else if args = line.match /^\s*([A-Z0-9]+)\s+([A-Za-z0-9]+)\s+(.+)\s*$/
  try url = new UrlAdapter(args[3])
  catch err then console.log "failed to parse URL: #{err}"

If the line starts with two words where first one is all-caps and second one is alphanumeric (no punctuation) we'll try to parse the third word as a URL.

UrlAdapter lets browsers use the built-in URL API while importing a similar library for the tests running node.js. I must not have tested a broad enough selection of node versions.

It might also be resolved by specifying The Right Version for URL library in package.json

[dobbs]

The test cases use the single capital letter A. That qualifies in the regex as a single, all-caps word. That's the first thing our regex matches.

If I change "A" to "About" in the testcases then the video displays because the parser recognizes the word as a comment and not a player designation.

I like your approach to mine the federation for test cases.

My first guess for refactoring the regexp would be to change it from /^\s*([A-Z0-9]+)\s+ to /^\s*([A-Z0-9]{3,})\s+ Or similar. I haven't tested that specific syntax... but I'm aiming at indicating that our first word has to be at least three upper case characters. That would still break if someone's comment starts a comment with FBI or CIA or other three letter agency.

[WardCunningham]

This seems like a good approach. Should I assemble up the test database I have suggested then I'll run all recent version and discuss the results here. Until then, might as well fix David's case.