Add rules: `no-dupe-keys` and `no-reserved-keys` #88

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.

Sign up for GitHub

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

Jump to bottom

Merged

michalsnik merged 1 commit into vuejs:master from armano2:patch-12-no-duplicate-field-names

Aug 1, 2017

Contributor

armano2 commented Jul 17, 2017 •

edited

Loading

This PR implements rules proposed in #86

DONE:

Create documtation
Add tests
Implement rule

Contributor Author

armano2 commented Jul 18, 2017

@michalsnik its ready to review

Member

michalsnik commented Jul 19, 2017

Let's do it one after another. It's really good to see you work so fast and being so committed! 💪 , however we didn't even approved the proposition yet. I hope you don't mind if we'll wait a while until merging all of your PRs? We'd like to focus and gracefully add more and more rules while constantly keeping everything well organized.

Contributor Author

armano2 commented Jul 19, 2017 •

edited

Loading

@michalsnik we are just need them in our project to improve quality of code.

armano2 force-pushed the patch-12-no-duplicate-field-names branch from 87fe19d to cecd4bf Compare

July 19, 2017 17:46

mysticatea added Ready for review semver-minor labels

armano2 changed the title ~~Add no-duplicate-field-names rule.~~ Add no-dupe-keys rule.

Member

michalsnik commented Jul 22, 2017

I added suggestion in #86

Contributor Author

armano2 commented Jul 22, 2017

@michalsnik rule splited to:

no-dupe-keys
no-reservered-keys

armano2 force-pushed the patch-12-no-duplicate-field-names branch from 8df0324 to f645e0b Compare

July 22, 2017 16:43

michalsnik suggested changes

View reviewed changes

lib/rules/no-dupe-keys.js Outdated

+                    if (usedNames.indexOf(name) !== -1) {
+                      context.report({
+                        node: node,
+                        message: "Duplicate key '{{name}}'.",

Member

michalsnik Jul 23, 2017

Duplicated

lib/rules/no-dupe-keys.js Outdated

+              module.exports = {
+                meta: {
+                  docs: {
+                    description: 'Prevent duplicate field names',

Member

michalsnik Jul 23, 2017

Prevents duplication of field names

lib/utils/index.js Outdated

@@ @@ -384,5 +426,48 @@ module.exports = { @@
                       cb(node.arguments[0])
                     }
                   }
+                },
+                iterateProperties (node, scopeNames, cb) {

Member

michalsnik Jul 23, 2017

Please add comments that will describe those new functions

Member

michalsnik Jul 23, 2017

Also, what do you think about calling scopeNames -> groups instead? Scope is actually the same so being semantically correct we'd call it rather a group. At least I think so

lib/rules/no-reservered-keys.js Outdated

+                const scopeNames = SCOPE_NAMES
+                const options = context.options[0] || {}
+                if (options.reserved) {

Member

michalsnik Jul 23, 2017

You can actually simplify config with:

const options = context.options[0] || {}
const reservedKeys = new Set([...RESERVED_KEYS, ...options.reserved || []])
const scopes = new Set([...SCOPE_NAMES, ...options.scopes || []])

Plus you can then set RESERVED_KEYS and SCOPE_NAMES as arrays above.
I changed names of variables so keep it in mind :)

Contributor Author

armano2 Jul 23, 2017

spread operator is not working with node 4

lib/rules/no-reservered-keys.js Outdated

+              // Rule Definition
+              // ------------------------------------------------------------------------------
+              const RESERVER_NAMES = new Set(require('../utils/vue-reserved.json'))

Member

michalsnik Jul 23, 2017

RESERVED, not RESERVER

lib/rules/no-reservered-keys.js Outdated

+                    if (groupName === 'data' && name.substr(0, 1) === '_') {
+                      context.report({
+                        node: node,
+                        message: "Field '{{name}}' which start with _ in data is reserved.",

Member

michalsnik Jul 23, 2017

What do you think about this message: Keys starting with with "_" are reserved in "data" group. ?

lib/rules/no-reservered-keys.js Outdated

+                    } else if (reservedNames.has(name)) {
+                      context.report({
+                        node: node,
+                        message: "Reserved key '{{name}}'.",

Member

michalsnik Jul 23, 2017

Key '{{name}}' is reserved

lib/rules/no-reservered-keys.js Outdated

+              module.exports = {
+                meta: {
+                  docs: {
+                    description: 'Prevent overwrite reserved keys',

Member

michalsnik Jul 23, 2017

I added suggestion for description above

lib/utils/index.js Outdated

+                },
+                iterateArrayExpression (node, groupName, cb) {
+                  node.elements.forEach(item => {

Member

michalsnik Jul 23, 2017

I'd check type of the passed node in each of those functions first.

tests/lib/rules/no-dupe-keys.js

+                          foo: String
+                        },
+                        computed: {
+                          ...mapGetters({

Member

michalsnik Jul 23, 2017

We don't support checking mapGetters and mapState case yet right? It's probably something worth considering but only once we have vuex environment available in this plugin.

Contributor Author

armano2 Jul 23, 2017 •

edited

Loading

@michalsnik we are not supporting it yet, and i'm waiting with it for respond about environments 💃 this is just example with spread operator.

michalsnik changed the title ~~Add no-dupe-keys rule.~~ Add rules: no-dupe-keys and no-reserved-keys

armano2 force-pushed the patch-12-no-duplicate-field-names branch from bfb41ef to 239dff6 Compare

July 23, 2017 14:23

Contributor Author

armano2 commented Jul 23, 2017

@michalsnik suggestions applied

mysticatea reviewed

View reviewed changes

lib/utils/index.js Outdated

@@ @@ -398,5 +440,69 @@ module.exports = { @@
                       cb(node.arguments.slice(-1)[0])
                     }
                   }
+                },
+                iterateProperties (node, groups, cb) {

Member

mysticatea Jul 23, 2017

There is the reason you avoid generator functions by callback style?
I like for-of.

Contributor Author

armano2 Jul 23, 2017

Node 4

Contributor Author

armano2 commented Jul 23, 2017

Node 4 :D

Member

mysticatea commented Jul 23, 2017

Node 4 supports generators and for-of 😄

Contributor Author

armano2 commented Jul 23, 2017

Aaaa I was almost 100% sure than it's not supported....

Than I can change it after launch

Contributor Author

armano2 commented Jul 23, 2017

@mysticatea updated

Member

mysticatea commented Jul 23, 2017

@armano2 Thank you for prompt update! I'm sorry that I give up today... (AM 1:30 of Monday in my timezone).

michalsnik approved these changes

View reviewed changes

armano2 force-pushed the patch-12-no-duplicate-field-names branch from 1347b26 to 270754e Compare

July 31, 2017 19:52

michalsnik reviewed

View reviewed changes

lib/utils/index.js

+                 * @param {ASTNode} node - The node to get.
+                 * @return {string|null} The property name if static. Otherwise, null.
+                 */
+                getStaticPropertyName (node) {

Member

michalsnik Aug 1, 2017

Can you please add test for this method?

Contributor Author

armano2 Aug 1, 2017

@michalsnik added


          Add rules: no-dupe-keys and no-reserved-keys.

56e1422

fixes vuejs#86

armano2 force-pushed the patch-12-no-duplicate-field-names branch from 270754e to 56e1422 Compare

August 1, 2017 11:33

michalsnik merged commit 39c9df5 into vuejs:master

armano2 deleted the patch-12-no-duplicate-field-names branch

August 1, 2017 14:19

filipalacerda pushed a commit to filipalacerda/eslint-plugin-vue that referenced this pull request


          Merge branch 'master' into 47-props-per-line

2f0492d

* master:
  Add rule `vue/require-valid-default-prop`. (vuejs#119)
  3.10.0
  Update readme to 3.10.0
  Chore: remove package-lock.json (vuejs#128)
  Fix: parserService must exist always (fixes vuejs#125) (vuejs#127)
  Add rule `require-render-return`. (vuejs#114)
  3.9.0
  Update package-lock
  Update: options for `no-duplicate-attributes` (fixes vuejs#112)(vuejs#113)
  New: add rule `attribute-hyphenation`. (fixes vuejs#92)(vuejs#95)
  Add namespace check of svg & mathML instead of tag names (vuejs#120)
  ⚠️ Add support for deprecated state in update-rules ⚠️ (vuejs#121)
  Add rules: `no-dupe-keys` and `no-reserved-keys`. (vuejs#88)
  Chore: Improve tests for name-property-casing & improve documentation (vuejs#115)
  New: add `require-prop-types` rule (fixes vuejs#19)(vuejs#85)
  Update: upgrade vue-eslint-parser (fixes vuejs#36, fixes vuejs#56, fixes vuejs#96) (vuejs#116)

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels