-
-
Notifications
You must be signed in to change notification settings - Fork 26
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
feat: add missing-directive-value
rule
#322
feat: add missing-directive-value
rule
#322
Conversation
🦋 Changeset detectedLatest commit: f6f48cb The changes in this PR will be included in the next version bump. This PR includes changesets to release 1 package
Not sure what this means? Click here to learn what changesets are. Click here if you're a maintainer who wants to add another changeset to this PR |
I noticed that I haven't written any instructions on how to contribute to this repository. https://github.com/ota-meshi/eslint-plugin-astro/blob/main/CONTRIBUTING.md |
5bba417
to
3373d4e
Compare
3373d4e
to
c84c87a
Compare
thanks for sharing that @ota-meshi! Sorry for my delayed reply, I think this PR is ready for your review, please. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thank you for this PR!
I think the rule name need to be a little more clear.
This rule targets the client:only
directive, so a name like missing-client-only-directive-value
would be a good. What do you think?
tests/fixtures/rules/missing-client-directive-value/valid/client-only-value.astro
Outdated
Show resolved
Hide resolved
tests/fixtures/rules/missing-client-directive-value/SomeSvelteComponent.svelte
Outdated
Show resolved
Hide resolved
Thanks for your review @ota-meshi ! I've pushed the changes mentioned after your review
That was my initial thought as well but then was thinking it to name it more generic in case in the future Astro team introduces a new directive that needs to satisfy the same condition. |
Hmm. I cannot agree with your opinion. Also, I like rule implementation to be as simple as possible, so I think it's better to create new rules for rules that report something different. |
fair points. I don't mind changing it so I'll go ahead and change it to |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM! Thank you for your contribution!
great! It was my pleasure :D |
According to Astro docs,
client:only
directive is required to have a value which represent the correct framework so I think it would be useful to have a rule about this to remind ourselves about this.Astro docs reference: https://docs.astro.build/en/reference/directives-reference/#clientonly