If syntax error message #16326
If syntax error message #16326
Conversation
There was a problem hiding this comment.
Looking good, thanks for picking this up!
I left some inline comments (mostly just moving from assert.throws to expectAssertion which should fix an issue with the production build), but would also like to see some tests added confirming a few additional scenarios work:
{{some-thing foo=(if)}}should error{{some-thing foo=(if foo bar baz)}}should not error{{if foo bar baz}}should not error
|
|
||
| moduleFor('ember-template-compiler: assert-if-helper-without-argument', class extends AbstractTestCase { | ||
| [`@test block if helper expects one argument`](assert) { | ||
| assert.throws(() => { |
There was a problem hiding this comment.
I believe this should be expectAssertion
| }); | ||
| }, `#if requires a single argument. ('baz/foo-bar' @ L1:C0) `); | ||
|
|
||
| assert.throws(() => { |
| } | ||
|
|
||
| [`@test inline if helper expects between one and three arguments`](assert) { | ||
| assert.throws(() => { |
| } | ||
|
|
||
| function isInvalidInlineIf(node) { | ||
| return node.path.original === 'if' && (!node.params || node.params.length < 1); |
There was a problem hiding this comment.
Should we make the minimum length be 2? I'm not sure how something like {{if something}} is useful (e.g. if there is no truthy or falsey argument, what is even happening?)...
What do you think @AlexTraher?
There was a problem hiding this comment.
@rwjblue Yeah agreed - I like the idea it fails loudly.
|
Looks great! Can you squash down to a single commit? |
…lude source of error
AlexTraher
force-pushed
the
alextraher/if-syntax-error-message
branch
from
db83e0e
to
2add803
Compare
|
Thanks for making the time to give feedback! 👍 |
This (hopefully) resolves issue 16322 regarding an unclear error message when passing an incorrect number of arguments to an
ifhelper.