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

close #928

This PR adds vue/no-use-computed-property-like-method rule.
https://deploy-preview-1234--eslint-plugin-vue.netlify.app/rules/no-use-computed-property-like-method.html

@tyankatsu0105 tyankatsu0105 marked this pull request as draft Jul 7, 2020
@tyankatsu0105 tyankatsu0105 force-pushed the feat/add-no-use-computed-property-like-method branch from d1fe2a1 to 6b0b860 Oct 10, 2020
@tyankatsu0105 tyankatsu0105 marked this pull request as ready for review Oct 10, 2020
@tyankatsu0105
Copy link
Contributor Author

@tyankatsu0105 tyankatsu0105 commented Oct 10, 2020

Sorry for my slow work😭

Loading

lib/rules/no-use-computed-property-like-method.js Outdated Show resolved Hide resolved
Loading
@tyankatsu0105
Copy link
Contributor Author

@tyankatsu0105 tyankatsu0105 commented Nov 10, 2020

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

Loading

@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
@tyankatsu0105 tyankatsu0105 requested a review from ota-meshi Nov 14, 2020
Copy link
Member

@ota-meshi ota-meshi left a comment

Sorry for the late reply. I posted some comments.

Loading

lib/rules/no-use-computed-property-like-method.js Outdated Show resolved Hide resolved
Loading
Loading
Loading
Loading
Loading
lib/rules/no-use-computed-property-like-method.js Outdated Show resolved Hide resolved
Loading
lib/rules/no-use-computed-property-like-method.js Outdated Show resolved Hide resolved
Loading
lib/rules/no-use-computed-property-like-method.js Outdated Show resolved Hide resolved
Loading
lib/rules/no-use-computed-property-like-method.js Outdated Show resolved Hide resolved
Loading
lib/rules/no-use-computed-property-like-method.js Outdated Show resolved Hide resolved
Loading
@tyankatsu0105 tyankatsu0105 requested a review from ota-meshi May 3, 2021
</script>
`
}
],
Copy link
Member

@ota-meshi ota-meshi May 11, 2021

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>

Loading

Copy link
Contributor Author

@tyankatsu0105 tyankatsu0105 Jun 19, 2021

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 :)

Loading

Copy link
Contributor Author

@tyankatsu0105 tyankatsu0105 Jul 17, 2021

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.

Loading

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.

Loading

Copy link
Contributor Author

@tyankatsu0105 tyankatsu0105 Jul 17, 2021

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

Loading

Copy link
Member

@ota-meshi ota-meshi left a comment

Looks good to me! Thank you!

Loading

@ota-meshi ota-meshi merged commit 9856621 into vuejs:master Jul 29, 2021
10 checks passed
Loading
@tyankatsu0105 tyankatsu0105 deleted the feat/add-no-use-computed-property-like-method branch Aug 2, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Linked issues

Successfully merging this pull request may close these issues.

2 participants