Add TAP component #1430
Add TAP component #1430
Conversation
Includes build changes.
|
@Golmote Not great at regex, so I'm struggling with getting yamlish working. Got an idea? |
|
I would say you need something like yamlish: {
pattern: /(^[^\S\r\n]*)---(?:\r\n?|\n)(?:.*(?:\r\n?|\n))*?[^\S\r\n]*\.\.\.$/m,
lookbehind: true,
inside: Prism.languages.yaml,
alias: 'language-yaml'
}The lookbehind (the first capturing group) matches the optional spaces before This should match: |
|
@Golmote I think we're looking good here. Let me know if you have suggestions. |
There was a problem hiding this comment.
Since you removed the (^|\n) parts that were checking for the beginning of the string in several regexps, I'm wondering if the ( )* parts are still needed. You might want to try removing them and see how the tests go.
Also, I commented inline on some more specific improvements.
Thanks for working on this!
components/prism-tap.js
Outdated
| @@ -0,0 +1,20 @@ | |||
| Prism.languages.tap = { | |||
| fail: /( )*not ok[^#\{\n]*/, | |||
| pass: /( )*ok[^#\{\n]*/, | |||
There was a problem hiding this comment.
You do not need to escape the { in the character class in those two regexps.
Also, we should probably include \r in the character class too, to handle the different line endings.
components/prism-tap.js
Outdated
| Prism.languages.tap = { | ||
| fail: /( )*not ok[^#\{\n]*/, | ||
| pass: /( )*ok[^#\{\n]*/, | ||
| pragma: /( )*pragma ([\+-])([a-z]+)/, |
There was a problem hiding this comment.
You do not need to escape the + in the character class.
Also, the last two pairs of parentheses are not needed.
components/prism-tap.js
Outdated
| pragma: /( )*pragma ([\+-])([a-z]+)/, | ||
| bailout: /( )*bail out!(.*)/i, | ||
| version: /( )*TAP version ([0-9]+)/i, | ||
| plan: /( )*([0-9]+)\.\.([0-9]+)( +#[^\n]*)?/m, |
There was a problem hiding this comment.
You can replace all occurrences of ([0-9]+) with \d+ (no parentheses needed here) in those two regexps.
Also, it seems the m could be removed, since the regexp does not use ^ or $ assertions.
Finally, the [^\n]* part could probably be replaced with .*.
components/prism-tap.js
Outdated
| version: /( )*TAP version ([0-9]+)/i, | ||
| plan: /( )*([0-9]+)\.\.([0-9]+)( +#[^\n]*)?/m, | ||
| subtest: { | ||
| pattern: /( )*# Subtest(?:: (.*))?/, |
There was a problem hiding this comment.
The parentheses around (.*) are not needed.
|
|
||
| ---------------------------------------------------- | ||
|
|
||
| Checks test pass & fail together correctly |
There was a problem hiding this comment.
Considering you added this test, I think it can be overkill to also have the pass_feature and fail_feature tests separately.
| @@ -0,0 +1,11 @@ | |||
| 1..10 | |||
There was a problem hiding this comment.
The plan pattern in the language definition seems to also include an optional directive (e.g. # foobar) after the numbers part. If this is what is intended, we should add a test for it here.
mAAdhaTTah
force-pushed
the
add-tap
branch
2 times, most recently
from
73f1f68
to
32120e5
Compare
|
@Golmote Updated. |
|
Looks quite good to me. There are still a few nitpicks, but I won't mind if this is merged as is. You did not give me feedback regarding this comment. Did you give it a try? Are those needed?
Also I can still see some unneeded parentheses:
Finally, all the remaining groups, expect in the |
|
I missed that comment the first time around. Made the changes suggested; tests still pass. Also added the minified file. |
Still need to add tests, but I squashed the commits from #1089.