-
-
Notifications
You must be signed in to change notification settings - Fork 387
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-dataset
rule
#225
Add prefer-dataset
rule
#225
Conversation
{ | ||
code: 'element.setAttribute(\'data-foo\', /* comment */ \'bar\');', | ||
errors, | ||
output: 'element.dataset.foo = \'bar\';' |
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.
I don't think we want to delete comments during fixing but it probably doesn't come up that often. @sindresorhus, what's your take on this?
- Removing comments is okay
- Comments should be preserved (may be non-trivial)
- Do not fix if comments are detected
I've done (3) in the past but I think a bunch of different rules current do (1).
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.
@MrHen I don't think we should do 1.
, but we need to come up with a guideline that we can apply to all rules. Maybe we could make an utility that just extract all comments to above a node we give it? The comments can require manual formatting and movement. We just shouldn't remove them. Alternatively, 3.
if 2.
is too complex, but I think we could make something generic that we could apply to many rules. Would you be able to open a new issue about it? We probably need to review all existing rules for this and update the contributing guidelines.
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, @amedora :) |
Note 850b8a1. I missed that during the review. |
Fix #166