-
-
Notifications
You must be signed in to change notification settings - Fork 4.2k
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
[BUGFIX beta] Change preference in positional parameters #12350
Conversation
|
||
for (let i = 0; i < positionalParams.length; i++) { | ||
let param = params[paramsStartIndex + i]; | ||
if (!(positionalParams[i] in attrs)) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think this should be positionalParams[paramsStartIndex + i]
.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Gotcha, thank you for explaining.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Can you make this an assertion though?
Basically:
assert(`You cannot specify both a positional param (at position ${i}) and the hash argument \`${positionalParams[i]}\`.`, positonalParams[i] in attrs);
Then the tests get updated to something like:
expectAssertion(function() {
runAppend(view);
}, /The message from above, I am too lazy to type/);
This fixes the behaviour when a positional param conflicts conflicts with a hash param.
3aa9f6e
to
9edd17f
Compare
[BUGFIX beta] Change preference in positional parameters
Thank you! |
Ideally we wouldn't need a separate `positionalIcon` property and could just use `icon`, but this is not permitted (see emberjs/ember.js#12350)
Ideally we wouldn't need a separate `positionalIcon` property and could just use `icon`, but this is not permitted (see emberjs/ember.js#12350)
This fixes the behaviour when a positional param conflicts conflicts with
a hash param.