Refactor directive parsing for code reuse #1242
Conversation
This removes the copy-pasta for directive parsing and removes the need for special if-else cases for each directive.
Codecov Report
@@ Coverage Diff @@
## master #1242 +/- ##
=========================================
+ Coverage 91.73% 91.9% +0.17%
=========================================
Files 126 126
Lines 4571 4557 -14
Branches 1493 1486 -7
=========================================
- Hits 4193 4188 -5
+ Misses 157 153 -4
+ Partials 221 216 -5
Continue to review full report at Codecov.
|
| @@ -24,7 +83,7 @@ function readExpression(parser: Parser, start: number, quoteMark: string|null) { | |||
| } else { | |||
| str += char; | |||
| } | |||
| } else if (/\s/.test(char)) { | |||
| } else if (/[\s\r\n\/>]/.test(char)) { | |||
There was a problem hiding this comment.
\s should match the same character group as [\s\r\n]...
Matches a single white space character, including space, tab, form feed, line feed and other Unicode spaces. Equivalent to [ \f\n\r\t\v\u00a0\u1680\u2000-\u200a\u2028\u2029\u202f\u205f\u3000\ufeff].
For example, /\s\w*/ matches " bar" in "foo bar".
Matches a single white space character, including space, tab, form feed, line feed and other Unicode spaces. Equivalent to [ \f\n\r\t\v\u00a0\u1680\u2000-\u200a\u2028\u2029\u202f\u205f\u3000\ufeff].For example, /\s\w*/ matches " bar" in "foo bar".
There was a problem hiding this comment.
ah whoops, missed this review before i hit merge, will account for these comments in a follow-up. sorry!
| @@ -1,5 +1,5 @@ | |||
| { | |||
| "message": "bound values should not be wrapped — use 'foo', not '{{foo}}'", | |||
| "message": "directive values should not be wrapped — use 'foo', not '{{foo}}'", | |||
There was a problem hiding this comment.
I'm not sure this tests the same thing. A directive is not the same thing as a bound value...
There was a problem hiding this comment.
Directives are special attributes with a colon. Bindings happen to be one type of directive, but all directive expressions should not be wrapped. This is a more general message for any directives. I could change it to use the specific name of the directive. E.g. Binding values should not be wrapped and EventHandler values should not be wrapped. But since the error points to the code where the issue is, I don't feel this provides much.
There was a problem hiding this comment.
Can see both sides of this, though I think it's probably ok to leave in —
on:click='{{set({foo: 'bar' })}}'
is just as incorrect as bind:value='{{foo}}', it's just less likely that someone would make that mistake.
| @@ -1,5 +1,5 @@ | |||
| { | |||
| "message": "Expected call expression", | |||
| "message": "Expected CallExpression", | |||
There was a problem hiding this comment.
I think this is less expressive than using English.
There was a problem hiding this comment.
I agree, it is more literal. Are you thinking we should write a method to humanize the expression type in the errors?
don't treat :foo as a directive
This removes the copy-pasta for directive parsing and removes the need for special if-else cases for each directive. Future directives (if any) will benefit from this cleanup as well.