Skip to content

Assert that readOnly isn't called over CPs with setter#10761

Merged
rwjblue merged 1 commit intoemberjs:masterfrom
cibernox:throw_error_on_readonly_properties_with_setter
Mar 30, 2015
Merged

Assert that readOnly isn't called over CPs with setter#10761
rwjblue merged 1 commit intoemberjs:masterfrom
cibernox:throw_error_on_readonly_properties_with_setter

Conversation

@cibernox
Copy link
Contributor

In preparation for emberjs/rfcs#12

Ensure that an error is thrown is readOnly is invoked over CP's that defines a setter function. It is conceptually absurd.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Needs to use expectAssertion (because prod build tests will not throw since asserts are removed from prod builds).

@cibernox cibernox force-pushed the throw_error_on_readonly_properties_with_setter branch 2 times, most recently from 39f3564 to 5b74634 Compare March 29, 2015 23:32
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This seems ok, but I don't understand what it is for...

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The last ok? I've seen similar lines in other tests related to assertions, but I agree that is redundant. Gone

@cibernox cibernox force-pushed the throw_error_on_readonly_properties_with_setter branch from 5b74634 to 1eeb925 Compare March 30, 2015 07:58
rwjblue added a commit that referenced this pull request Mar 30, 2015
…rties_with_setter

Assert that readOnly isn't called over CPs with setter
@rwjblue rwjblue merged commit e0a31ea into emberjs:master Mar 30, 2015
@rwjblue
Copy link
Member

rwjblue commented Mar 30, 2015

Thank you!

@tomdale
Copy link
Member

tomdale commented Apr 27, 2015

This should not generate an exception, as this is a breaking change that causes apps that happened to have an extra unused argument in their CP functions to break. We can deprecate it in 1.x and cause it to raise in 2.x.

@cibernox
Copy link
Contributor Author

I haven't considered that, you're right, it's beaking.
However, I think I can fix it by adding to the assertion that the getter and the setter are not the same function. That way the assert will not be fired with the old syntax.

@cibernox cibernox deleted the throw_error_on_readonly_properties_with_setter branch April 27, 2015 20:46
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants