-
-
Notifications
You must be signed in to change notification settings - Fork 681
Add prefer-true-attribute-shorthand
rule
#1796
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
Add prefer-true-attribute-shorthand
rule
#1796
Conversation
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!
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.
Thanks for implementing this! However, I have a few remarks.
In #775 (comment), it was suggested to make it configurable whether to always or never use shorthand props. I think that's a good idea, given some people may prefer to never use shorthand props because of their quirks or just by taste.
The never
case could also be auto-fixable, while the suggestion you implemented would be fine for the always
case.
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.
Nice, this looks really good!
Only one problem I just noticed: The never
option will also report native HTML shorthand attributes: <input checked>
should become <input checked="checked">
, not <input :checked="true">
. Or is this actually allowed?
If it's allowed, please also add this test case to the "allowed" list to document this behavior.
If it isn't, we need a little more logic: Since one can use native HTML shorthand attributes like hidden
also on components (e.g. <MyComponent hidden>
), I think we also have to use suggestions instead of an autofix there: One for changing it into a longhand Vue prop, one for changing it into a longhand HTML attribute.
We might be a bit more clever about this and actually do autofix it in one case though: The shorthand attribute is on a known HTML element. Then we know that it should be autofixed to a longhand HTML attribute. But I think this is a nice-to-have improvement; only having suggestions in all cases would be fine for me.
What about skipping native HTML elements, and just check components? |
That'd be fine for me, but still not catch the |
We can turn auto fix into suggestions, as you said. |
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.
Nice, thanks for the improvements and the test cases! Please run npm run update
one more time to update the docs (there is no autofix anymore). But this is not merge-blocking; we can do it afterwards.
Closes #775
Closes #1781