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.

Sign up for GitHub

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

Jump to bottom

Add new 'require-return-from-computed' rule #262

Merged
merged 2 commits into from
Mar 16, 2019
Merged

Add new 'require-return-from-computed' rule #262

merged 2 commits into from
Mar 16, 2019

Conversation

gmurphey
Copy link
Contributor

Fixes #247.

@gmurphey gmurphey force-pushed the return-from-computed branch 3 times, most recently from e55b214 to 50737f1 Compare June 12, 2018 00:16
@Turbo87
Copy link
Member

Turbo87 commented Jun 28, 2018

@gmurphey please don't add rules to the recommended list directly. adding them means it is a breaking change and we would have to release a new major version each time.

@rwjblue rwjblue mentioned this pull request Jun 28, 2018
Merged
@gmurphey gmurphey force-pushed the return-from-computed branch from 50737f1 to fd1857c Compare July 9, 2018 18:22
@gmurphey
Copy link
Contributor Author

gmurphey commented Jul 9, 2018

Thanks for the heads up, @Turbo87. I've reverted those changes.

@gmurphey gmurphey force-pushed the return-from-computed branch from fd1857c to a68c854 Compare July 9, 2018 18:30
Turbo87
Turbo87 reviewed Jul 10, 2018
View reviewed changes
tests/lib/rules/return-from-computed.js Outdated
}]
},
{
code: 'let foo = computed("test", function() { if (true) { return ""; } })',
Copy link
Member

Choose a reason for hiding this comment

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

why is this check invalid? it seems correct to me as there is a return statement inside the function 🤔

Copy link
Member

Choose a reason for hiding this comment

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

@gmurphy any comments?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Sorry for the long radio silence. Since not all paths return, I thought this should be an invalid case. I've added a valid case for this example as well.

@gmurphey gmurphey force-pushed the return-from-computed branch from a68c854 to 517d65f Compare November 12, 2018 20:28
@gmurphey
Copy link
Contributor Author

I've updated this to use some of the logic from the getter-return ESLint rule, since it's very close to what we want.

@gmurphey gmurphey force-pushed the return-from-computed branch 4 times, most recently from b2323a6 to cad1aad Compare November 13, 2018 15:35
@Turbo87 Turbo87 changed the title Adding return-from-computed recommended rule Adding return-from-computed rule Nov 15, 2018
rwjblue
rwjblue reviewed Jan 4, 2019
View reviewed changes
Copy link
Member

@rwjblue rwjblue left a comment

Choose a reason for hiding this comment

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

Can you add tests for the following;

let foo = computed({
  get() {
    return 'foo';
  },
  set() {
  }
})
let foo = computed({
  get() {
  },
  set() {
    return 'foo';
  }
})

Other than that, this seems good to me...

@bmish
Copy link
Member

bmish commented Jan 5, 2019

Suggestion: rename rule to require-return-from-computed for improved clarity / consistency with other rule names.

@rwjblue
Copy link
Member

rwjblue commented Jan 11, 2019

Rename suggestion seems good to me...

@Alonski
Copy link
Contributor

Alonski commented Mar 10, 2019

Hey everyone,
I ran into this issue today. What would it take to get this merged?

@gmurphey
Copy link
Contributor Author

I should have some time this week to make the requested changes. Sorry for the delay!

@bmish
Copy link
Member

bmish commented Mar 15, 2019

Hey @gmurphey I can help you finish this up and merge it if you prefer.

@gmurphey gmurphey force-pushed the return-from-computed branch from cad1aad to 382e55c Compare March 16, 2019 13:08
@gmurphey
Copy link
Contributor Author

Thanks for offering, @bmish! Was able to find a few minutes this morning to rename the rule and add the requested tests. This should be ready for another round of review. Thanks everyone for the feedback so far!

bmish
bmish reviewed Mar 16, 2019
View reviewed changes
tests/lib/rules/require-return-from-computed.js Outdated
// ------------------------------------------------------------------------------

const eslintTester = new RuleTester();
eslintTester.run('return-from-computed', rule, {
Copy link
Member

Choose a reason for hiding this comment

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

Update rule name here.

bmish
bmish reviewed Mar 16, 2019
View reviewed changes
lib/rules/require-return-from-computed.js Outdated
description: 'Warns about missing return statements in computed properties',
category: 'Possible Errors',
recommended: false,
url: 'https://github.com/ember-cli/eslint-plugin-ember/tree/master/docs/rules/no-side-effects.md'
Copy link
Member

Choose a reason for hiding this comment

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

This is the wrong URL. I'm not sure if this url property is actually used anywhere so you could just remove it.

@bmish
Copy link
Member

bmish commented Mar 16, 2019

Looks like you need to run yarn update inside the repository still which will add this rule to README.md and to lib/recommended-rules.js (as off).

@bmish bmish changed the title Adding return-from-computed rule Add new 'require-return-from-computed' rule Mar 16, 2019
@gmurphey gmurphey force-pushed the return-from-computed branch from 382e55c to 557380e Compare March 16, 2019 19:35
@bmish bmish self-requested a review March 16, 2019 20:38
@bmish bmish merged commit 4434b86 into ember-cli:master Mar 16, 2019
@bmish
Copy link
Member

bmish commented Mar 16, 2019

Thank you!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@rwjblue rwjblue rwjblue left review comments

@Turbo87 Turbo87 Turbo87 left review comments

@bmish bmish bmish approved these changes

Assignees
No one assigned
Labels
enhancement
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
5 participants
@gmurphey @Turbo87 @bmish @rwjblue @Alonski