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

New: add no-use-computed-property-like-method rules #1234

Conversation

tyankatsu0105
Copy link
Contributor

@tyankatsu0105 tyankatsu0105 commented Jul 7, 2020

@tyankatsu0105 tyankatsu0105 marked this pull request as draft July 7, 2020 14:14
@tyankatsu0105 tyankatsu0105 force-pushed the feat/add-no-use-computed-property-like-method branch from d1fe2a1 to 6b0b860 Compare October 10, 2020 01:18
@tyankatsu0105 tyankatsu0105 marked this pull request as ready for review October 10, 2020 02:46
@tyankatsu0105
Copy link
Contributor Author

Sorry for my slow work😭

@tyankatsu0105
Copy link
Contributor Author

tyankatsu0105 commented Nov 10, 2020

@ota-meshi
Probably, I can use executeOnVue for this rule, right?
Sorry, please ignore

@tyankatsu0105 tyankatsu0105 changed the title feat: add no-use-computed-property-like-method rules New: add no-use-computed-property-like-method rules Nov 14, 2020
Copy link
Member

@ota-meshi ota-meshi left a comment

Choose a reason for hiding this comment

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

Sorry for the late reply. I posted some comments.

lib/rules/no-use-computed-property-like-method.js Outdated Show resolved Hide resolved
lib/rules/no-use-computed-property-like-method.js Outdated Show resolved Hide resolved
lib/rules/no-use-computed-property-like-method.js Outdated Show resolved Hide resolved
lib/rules/no-use-computed-property-like-method.js Outdated Show resolved Hide resolved
lib/rules/no-use-computed-property-like-method.js Outdated Show resolved Hide resolved
lib/rules/no-use-computed-property-like-method.js Outdated Show resolved Hide resolved
</script>
`
}
],
Copy link
Member

Choose a reason for hiding this comment

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

I have found an error in some cases. Could you add the following test cases?

<script>
export default {
  computed: {
    bar() {
      return
    }
  }
}
</script>
<script>
export default {
  methods: {
    fn() {
      this.foo()
    }
  }
}
</script>

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Thanks!
I'll do that :)

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@ota-meshi
I changed some codes to support the above cases.
3b77c4e
ce93fa6

However, I will not support this case:

<script>
export default {
  methods: {
    fn() {
      this.foo()
    }
  }
}
</script>

Because this rule only checkes whether computed properties are expected as property or not.
In this case, computed is not used.

Copy link
Member

@ota-meshi ota-meshi Jul 17, 2021

Choose a reason for hiding this comment

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

I don't want support. I've found that using a rule in that source code reports a crash or an incorrect error, so I just want to add test cases to tests if it's okay.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Thanks.
I just make sense of what you said :)
I added test case
7cb18fe

Copy link
Member

@ota-meshi ota-meshi left a comment

Choose a reason for hiding this comment

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

Looks good to me! Thank you!

@ota-meshi ota-meshi merged commit 9856621 into vuejs:master Jul 29, 2021
@tyankatsu0105 tyankatsu0105 deleted the feat/add-no-use-computed-property-like-method branch August 2, 2021 14:42
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.

Rule Proposal: Prohibit calling computed properties like methods
2 participants