Add `return-in-computed-property` rule. #78

armano2 · 2017-07-15T16:21:57Z

This PR implements rules proposed in #75

DONE:

Basic check for return
Check if all paths return value
Option: treatUndefinedAsUnspecified - disallows implicitly returning undefined with a return; statement.

armano2 · 2017-07-16T11:55:39Z

@michalsnik can you review this pull request?

michalsnik · 2017-07-16T12:34:08Z

Sure, later today I'll try to push my PR and both of yours forward. Busy time :)

armano2 · 2017-07-16T15:32:56Z

It's my first time working with rules from eslint (developing them) but they seems not to complex to make.

sorry for pushing you to test my code 🗡

michalsnik

I'll continue checking the rest once my PR that serves as the base for all of yours is merged so the diff is more obvious :)

michalsnik · 2017-07-16T21:16:14Z

docs/rules/computed-property-return.md

@@ -0,0 +1,44 @@
+# Enforces that a return statement is present in computed property (computed-property-return)


I'd call this rule return-in-computed-property to make it more descriptive. WDYT?

i'm bad at naming stuff, your name is way better

michalsnik · 2017-07-16T21:19:32Z

docs/rules/computed-property-return.md

+- `"treatUndefinedAsUnspecified"`: `true` (default) allows implicitly returning undefined with a `return;` statement.
+
+```
+vue/order-in-components: [2, {


michalsnik · 2017-07-16T21:27:58Z

docs/rules/computed-property-return.md

+## :wrench: Options
+
+This rule has an object option:
+- `"treatUndefinedAsUnspecified"`: `true` (default) allows implicitly returning undefined with a `return;` statement.


I think we should force to return something explicitly, thus in my opinion return; should also trigger an error and this option should not be necessary.
We could consider two types of messages:

There is no return statement inside foo computed property

There is an incomplete return statement inside foo computed property
What do you think @armano2 @chrisvfritz ?

damn description is invalid, i forgot to change it, by default its treating undefined as unspecified than return; will thrown an error, but i prefer to leave a choice to disable it because undefined can be useful in some cases.

michalsnik · 2017-07-16T21:35:18Z

lib/rules/computed-property-return.js

+        }
+      }
+    },
+    utils.executeOnVueInstance(context, properties => {


Shouldn't it be executeOnVueComponent ? And this one in computed-data rule instead?

no right now there is 2 functions, executeOnVueInstance and executeOnVueComponent we have to distinct both of those types,

see: 281f230

Vue Component ~ Vue Instance

Yeah, but I thought this matters for data as function case, as it's ok to have data as object in vue instance, but not in component. And this rule is about return statement in computed property which seems irrelevant.

executeOnVueInstance supports components & new vue

i just renamed function to simplified merges and improve readability.

armano2 · 2017-07-19T16:07:53Z

@michalsnik cleaned up and merged

chrisvfritz · 2017-07-19T16:49:00Z

Will this also flag computed properties that technically have a return somewhere in their content, but not directly? For example:

computed: {
  foo: function () {
    function bar () {
      return this.baz * 2 
    }
    bar()
  }
}

armano2 · 2017-07-19T17:06:43Z

@chrisvfritz yes (test for this case added)

i think i should make distinction in messages based on hasReturn this flag is set when there is ReturnStatement in function but not all path return values

mysticatea · 2017-07-19T22:36:28Z

tests/lib/rules/return-in-computed-property.js

+        message: 'Expected to return a value in "foo" computed property.',
+        line: 4
+      }]
+    }


Could you add invalid tests for treatUndefinedAsUnspecified option?

@mysticatea added

michalsnik

Great work @armano2 !

armano2 changed the title ~~WIP: Patch 6 computed property return~~ WIP: Computed property return Jul 15, 2017

armano2 changed the title ~~WIP: Computed property return~~ WIP: Add computed-property-return rule. Jul 15, 2017

armano2 changed the title ~~WIP: Add computed-property-return rule.~~ Add computed-property-return rule. Jul 16, 2017

armano2 force-pushed the patch-6-computed-property-return branch from 86d85c6 to dbb36d8 Compare July 16, 2017 11:54

michalsnik added the Ready for review label Jul 16, 2017

michalsnik suggested changes Jul 16, 2017

View reviewed changes

michalsnik reviewed Jul 16, 2017

View reviewed changes

armano2 changed the title ~~Add computed-property-return rule.~~ Add return-in-computed-property rule. Jul 16, 2017

michalsnik added the do not merge label Jul 16, 2017

Add return-in-computed-property rule.

253a86e

armano2 force-pushed the patch-6-computed-property-return branch from 12eb0f1 to 253a86e Compare July 19, 2017 16:06

Add unit test proposed by @chrisvfritz

722f810

mysticatea reviewed Jul 19, 2017

View reviewed changes

Fix issue with treatUndefinedAsUnspecified & add missing unit test

59fd6a4

michalsnik removed the do not merge label Jul 21, 2017

michalsnik approved these changes Jul 21, 2017

View reviewed changes

michalsnik merged commit b7e6ff1 into vuejs:master Jul 21, 2017

armano2 deleted the patch-6-computed-property-return branch July 22, 2017 12:12

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add `return-in-computed-property` rule. #78

Add `return-in-computed-property` rule. #78

armano2 commented Jul 15, 2017 •

edited

Loading

armano2 commented Jul 16, 2017

michalsnik commented Jul 16, 2017

armano2 commented Jul 16, 2017

michalsnik left a comment

michalsnik Jul 16, 2017

armano2 Jul 16, 2017

michalsnik Jul 16, 2017

michalsnik Jul 16, 2017

armano2 Jul 16, 2017

michalsnik Jul 16, 2017

armano2 Jul 16, 2017 •

edited

Loading

michalsnik Jul 16, 2017

armano2 Jul 16, 2017

armano2 commented Jul 19, 2017

chrisvfritz commented Jul 19, 2017

armano2 commented Jul 19, 2017 •

edited

Loading

mysticatea Jul 19, 2017

armano2 Jul 19, 2017

armano2 Jul 19, 2017

michalsnik left a comment

		@@ -0,0 +1,44 @@
		# Enforces that a return statement is present in computed property (computed-property-return)

Add return-in-computed-property rule. #78

Add return-in-computed-property rule. #78

Conversation

armano2 commented Jul 15, 2017 • edited Loading

armano2 commented Jul 16, 2017

michalsnik commented Jul 16, 2017

armano2 commented Jul 16, 2017

michalsnik left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

armano2 Jul 16, 2017 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

armano2 commented Jul 19, 2017

chrisvfritz commented Jul 19, 2017

armano2 commented Jul 19, 2017 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

michalsnik left a comment

Choose a reason for hiding this comment

Add `return-in-computed-property` rule. #78

Add `return-in-computed-property` rule. #78

armano2 commented Jul 15, 2017 •

edited

Loading

armano2 Jul 16, 2017 •

edited

Loading

armano2 commented Jul 19, 2017 •

edited

Loading