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

refactor: store the defineEmits variable name #2592

Open
wants to merge 1 commit into
base: master
Choose a base branch
from

Conversation

waynzh
Copy link
Contributor

@waynzh waynzh commented Nov 2, 2024

related to #2585

Refactored to store the defineEmits variable name for checking defineEmits variable name usage in the template.

Copy link
Member

@FloEdelmann FloEdelmann left a comment

Choose a reason for hiding this comment

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

Nice! Does this add support for other cases previously not covered? If so, could you add tests for those please?

Otherwise, the refactoring looks fine to me.

@waynzh
Copy link
Contributor Author

waynzh commented Nov 5, 2024

yes, can check out here no-unused-emit-declarations L105 + require-explicit-emits L474

@FloEdelmann
Copy link
Member

I'm confused. It sounds like there are behavior changes the rules? But none of the existing tests fail, so the behavior changes should be covered by new tests.

@waynzh
Copy link
Contributor Author

waynzh commented Nov 5, 2024

Does this add support for other cases previously not covered

Sry, I mean NO 🤖, this does not add support for any additional cases.
It is purely a refactor of the existing code, as I found the previous implementation to be somewhat redundant and wanted to keep it consistent with this #2585.

Copy link
Member

@FloEdelmann FloEdelmann left a comment

Choose a reason for hiding this comment

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

Ah, I see. Thanks for the clarification and for the refactoring! LGTM then.

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.

2 participants