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

fix(eslint-plugin-vue): [valid-v-bind-sync] Added Case: when valid is then valid .sync #1335

Merged
merged 6 commits into from Dec 4, 2020

Conversation

aggmoulik
Copy link
Contributor

Fixes #1298
Signed-off-by: Moulik Aggarwal qwertymoulik@gmail.com

… Valid

Signed-off-by: Moulik Aggarwal <qwertymoulik@gmail.com>
Signed-off-by: Moulik Aggarwal <qwertymoulik@gmail.com>
@aggmoulik aggmoulik changed the title fix(eslint-plugin-vue): [valid-v-bind-sync] Added Case for Valid Is the Valid .sync fix(eslint-plugin-vue): [valid-v-bind-sync] Added Case: when valid is then valid .sync Oct 17, 2020
@aggmoulik aggmoulik changed the title fix(eslint-plugin-vue): [valid-v-bind-sync] Added Case: when valid is then valid .sync fix(eslint-plugin-vue): [valid-v-bind-sync] Added Case: when valid is then valid .sync Oct 17, 2020
Copy link
Member

@ota-meshi ota-meshi left a 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 have some change requests.

lib/rules/valid-v-bind-sync.js Outdated Show resolved Hide resolved
lib/rules/valid-v-bind-sync.js Outdated Show resolved Hide resolved
lib/rules/valid-v-bind-sync.js Outdated Show resolved Hide resolved
Signed-off-by: Moulik Aggarwal <qwertymoulik@gmail.com>
Signed-off-by: Moulik Aggarwal <qwertymoulik@gmail.com>
@aggmoulik
Copy link
Contributor Author

Hy @ota-meshi I have updated your changes and added a cli utility also, as it helps us to check individual rules and makes our development faster.

@aggmoulik
Copy link
Contributor Author

aggmoulik commented Oct 19, 2020

Also, Can you please add hacktoberfest label in the PR or in the repository tags. @ota-meshi

@ota-meshi
Copy link
Member

I'm currently maintaining this repository almost alone, so I can't afford to accept all PR during the hacktoberfest, so I won't add hacktoberfest label.
Also, I'm sorry, this PR can't promise to complete all reviews and merge in October.

@aggmoulik
Copy link
Contributor Author

@ota-meshi I think I can help you in that as I am working to be a active-contributor not for just hacktoberfest. I can help in code review as I already do in my company.

Copy link
Member

@ota-meshi ota-meshi left a comment

Choose a reason for hiding this comment

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

Sorry for the late reply. I posted some comments.

if (attr.key.type === 'VIdentifier' && attr.key.name === 'is') return true

// check for `:is` `bind` attribute
if (attr.key.type === 'VDirectiveKey' && attr.key.argument.name === 'is')
Copy link
Member

Choose a reason for hiding this comment

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

You need to check if it is v-bind. Also, the argument can be null.

https://github.com/vuejs/vue-eslint-parser/blob/master/docs/ast.md#vdirectivekey

In addition, could you add a test to make sure it is not judged as is?

<tr v-on:is="myRow" :some-prop.sync="somePropValue">
<tr v-bind="myRow" :some-prop.sync="somePropValue">

package.json Outdated
@@ -19,7 +19,8 @@
"version": "npm run lint -- --fix && git add .",
"update": "node ./tools/update.js",
"docs:watch": "vuepress dev docs",
"docs:build": "vuepress build docs"
"docs:build": "vuepress build docs",
"tests": "mocha"
Copy link
Member

Choose a reason for hiding this comment

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

Do not change the script. You may be able to use npx mocha.

tests/lib/rules/valid-v-bind-sync.js Outdated Show resolved Hide resolved
package.json Outdated Show resolved Hide resolved
@ota-meshi
Copy link
Member

I would like to share the fix related to this issue with another fix. So I'll fix this PR and merge it.

@ota-meshi ota-meshi merged commit af5c4c0 into vuejs:master Dec 4, 2020
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.

"vue/valid-v-bind-sync" is not accounting for "is" attribute
2 participants