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 attribute-hyphenation ignore lists #513

Merged
merged 4 commits into from
Jul 14, 2018

Conversation

LukeShu
Copy link
Contributor

@LukeShu LukeShu commented Jul 7, 2018

Commit f9e1eee introduced the "ignore" config option for the attribute-hyphenation rule. However, that commit has a few problems:

  • The added testcase doesn't actually test the functionality at all
  • The code only works correctly if the ignore list has exactly 1 item in it
  • There are spelling mistakes in the test

See each individual commit message for more details.

The test introduced in f9e1eee didn't
actually test what it meant to test for.  It attempts to verify that
the ignore list is obeyed; but its subject is an element that is
ignored in its entirety!  The code under test was never triggered!

Fix this by changing <div> for <custom> in the test case, and also add
the same test-case without the `ignore` config, in order to verify
that it actually has something to ignore.
@LukeShu LukeShu changed the title Fix attribute-hyphenation Fix attribute-hyphenation ignore lists Jul 7, 2018
@@ -46,7 +46,7 @@ ruleTester.run('attribute-hyphenation', rule, {
{
filename: 'test.vue',
code: '<template><custom data-id="foo" aria-test="bar" slot-scope="{ data }" custom-hypen="foo"><a onClick="" my-prop="prop"></a></custom></template>',
options: ['never', { 'ignore': ['custom-hypen'] }]
options: ['never', { 'ignore': ['custom-hypen', 'second-custom'] }]
Copy link
Member

Choose a reason for hiding this comment

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

Can you add second-custom attribute in above html and also add extra failing test with same ignore list, but only with one attribute in HTML?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I'm not entirely sure what you're asking for for the failing test case. Is this what you're looking for?

    {
      filename: 'test.vue',
      code: '<template><custom data-id="foo" aria-test="bar" slot-scope="{ data }" custom-hypen="foo" third-custom="bar"><a onClick="" my-prop="prop"></a></custom></template>',
      output: '<template><custom data-id="foo" aria-test="bar" slot-scope="{ data }" custom-hypen="foo" thirdCustom="bar"><a onClick="" my-prop="prop"></a></custom></template>',
      options: ['never', { 'ignore': ['custom-hypen', 'second-custom'] }],
      errors: [{
        message: "Attribute 'third-custom' cann't be hyphenated.",
        type: 'VDirectiveKey',
        line: 1
      }]
    },

@michalsnik
Copy link
Member

Thanks for working on it @LukeShu! I added one comment :)

@michalsnik michalsnik added this to the v5.0.0 milestone Jul 11, 2018
Currently, the "ignore" options array only works correctly with exactly 1
item.  Adjust it to work with 0 and with >1 items.
 - hypen -> hyphen
 - cann't -> can't
@LukeShu
Copy link
Contributor Author

LukeShu commented Jul 11, 2018

v2:

  • Make requested changes to the tests

The change from v1 to v2 is as follows:

diff --git a/tests/lib/rules/attribute-hyphenation.js b/tests/lib/rules/attribute-hyphenation.js
index 70d798f..ce659cc 100644
--- a/tests/lib/rules/attribute-hyphenation.js
+++ b/tests/lib/rules/attribute-hyphenation.js
@@ -45,7 +45,7 @@ ruleTester.run('attribute-hyphenation', rule, {
     },
     {
       filename: 'test.vue',
-      code: '<template><custom data-id="foo" aria-test="bar" slot-scope="{ data }" custom-hyphen="foo"><a onClick="" my-prop="prop"></a></custom></template>',
+      code: '<template><custom data-id="foo" aria-test="bar" slot-scope="{ data }" custom-hyphen="foo" second-custom="bar"><a onClick="" my-prop="prop"></a></custom></template>',
       options: ['never', { 'ignore': ['custom-hyphen', 'second-custom'] }]
     }
   ],
@@ -128,6 +128,17 @@ ruleTester.run('attribute-hyphenation', rule, {
         line: 1
       }]
     },
+    {
+      filename: 'test.vue',
+      code: '<template><custom data-id="foo" aria-test="bar" slot-scope="{ data }" custom-hyphen="foo" third-custom="bar"><a onClick="" my-prop="prop"></a></custom></template>',
+      output: '<template><custom data-id="foo" aria-test="bar" slot-scope="{ data }" custom-hyphen="foo" thirdCustom="bar"><a onClick="" my-prop="prop"></a></custom></template>',
+      options: ['never', { 'ignore': ['custom-hyphen', 'second-custom'] }],
+      errors: [{
+        message: "Attribute 'third-custom' can't be hyphenated.",
+        type: 'VDirectiveKey',
+        line: 1
+      }]
+    },
     {
       // This is the same code as the `'ignore': ['custom-hyphen']`
       // valid test; to verify that setting ignore actually makes the

@michalsnik
Copy link
Member

Thanks @LukeShu ! I fixed the test and also added some missing scenarios

@michalsnik michalsnik merged commit 8027916 into vuejs:master Jul 14, 2018
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants