Skip to content
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

Update reportUnorderedProperties util function to also work with native classes #574

Merged
merged 5 commits into from Nov 7, 2019

Conversation

laurmurclar
Copy link
Contributor

This is needed to unblock native class compatibility for the order-in-* rules, which we want to fix (see #417)

The reportUnorderedProperties function relied on a bunch of utils which currently only work for "classic" classes. This PR adds functionality which also make them compatible with native classes.

export default Component.extend({
levelOfHappiness: observer("attitude", "health", () => {
}),
vehicle: alias("car")
Copy link
Contributor

Choose a reason for hiding this comment

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

Is the vehicle prop here necessary for the test?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

🤦‍♀ Nope! Copy-pasta. Will update


it("should check if it's an observer prop", () => {
node = parse('observer()');
context = new FauxContext(`
Copy link
Contributor

Choose a reason for hiding this comment

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

What do you think about splitting each of these different "types" into their one test cases? That would help make the test output more clear about where something is going wrong, if one of these cases no longer worked right.

Copy link
Member

Choose a reason for hiding this comment

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

Yeah, it would generally be nice to have more, smaller unit tests.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Sounds good

Copy link
Member

@bmish bmish left a comment

Choose a reason for hiding this comment

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

A few minor comments and we can merge this soon!

lib/utils/property-order.js Show resolved Hide resolved

it("should check if it's an observer prop", () => {
node = parse('observer()');
context = new FauxContext(`
Copy link
Member

Choose a reason for hiding this comment

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

Yeah, it would generally be nice to have more, smaller unit tests.

Copy link
Member

@bmish bmish left a comment

Choose a reason for hiding this comment

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

Great, thank you! Can you fix the conflict due to a file being renamed? Then I will merge it.

@bmish bmish changed the title Update reportUnorderedProperties to also work with native classes Update reportUnorderedProperties util function to also work with native classes Nov 7, 2019
@laurmurclar
Copy link
Contributor Author

@bmish Done!

@bmish bmish merged commit 13d893f into ember-cli:master Nov 7, 2019
@laurmurclar laurmurclar deleted the lmc/native-order-in branch November 7, 2019 23:08
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants