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

Add rule attribute-hyphenation. #95

Merged
merged 2 commits into from
Aug 3, 2017

Conversation

armano2
Copy link
Contributor

@armano2 armano2 commented Jul 18, 2017

This PR implements rules proposed in #92

TODO:

This PR contains changes proposed in #120 and should be merged after it.

@armano2 armano2 changed the title Add rule html-attributes-casing. WIP: Add rule html-attributes-casing. Jul 20, 2017
@armano2
Copy link
Contributor Author

armano2 commented Jul 20, 2017

Based on #92 (comment) we are missing some cases

@armano2 armano2 force-pushed the patch-15-html-attributes-casing branch 2 times, most recently from c248c96 to a4be85d Compare July 23, 2017 01:11
@armano2 armano2 changed the title WIP: Add rule html-attributes-casing. Add rule html-attributes-casing. Jul 23, 2017
@armano2
Copy link
Contributor Author

armano2 commented Jul 23, 2017

@michalsnik rule is done

@armano2 armano2 force-pushed the patch-15-html-attributes-casing branch from 6178486 to 24305db Compare July 23, 2017 03:35
@michalsnik michalsnik requested a review from mysticatea July 23, 2017 13:41
@armano2 armano2 force-pushed the patch-15-html-attributes-casing branch from 24305db to 50dce57 Compare July 24, 2017 23:18
@@ -0,0 +1,35 @@
# Define a style for the attributes casing in templates. (html-attributes-casing)
Copy link
Member

@mysticatea mysticatea Aug 1, 2017

Choose a reason for hiding this comment

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

Thank you for the contributing! I'm sorry for the delay.

I'm not sure whether this rule makes sense. Below is the result of my investigation.

  • In all known HTML/SVG/MathML elements, all of attrname, attrName and AttrName are the same attribute but attr-name is another.
  • In custom components,
    • about defined properties,
      • for example attr-name property, people can choose attrName or attr-name but attrname and AttrName do not work.
      • for example attrName property, people can choose attrName or attr-name but attrname and AttrName do not work.
      • for example AttrName property, people can choose AttrName or attr-name but attrname and attrName do not work.
    • about undefined properties, the same behaivor to known HTML/SVG/MathML elements.

So I think the options camelCase and PascalCase are not useful because it might be uncontrollable due to the definition of libraries. So maybe.... What do you think below?

  • Change rule name to attribute-hyphenation
  • Options are: "always" (default) or "never".
    • "always" ... Use hyphenated name. (It errors on upper case letters.)
    • "never" ... Don't use hyphenated name. (It errors on hyphens except data- and aria-.)

(I think this rule don't need html- prefix because this rule can work on AST. I think that rules which use details of tokens should have html- prefix.)

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 like it:

  1. change name of rule to attribute-hyphenation
  2. apply rule only on components isCustomComponent
  3. change options to always (default) or never
  4. add aria- to exceptions

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@mysticatea there is bug: vuejs/vue-eslint-parser#12 with parsing argument for VDirectiveKey

// ----------------------------------------------------------------------

utils.registerTemplateBodyVisitor(context, {
'VStartTag' (obj) {
Copy link
Member

Choose a reason for hiding this comment

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

I think that you can use VAttribute handler to avoid extra looping.


utils.registerTemplateBodyVisitor(context, {
'VStartTag' (obj) {
if (!utils.isSvgElementName(obj.id.name) && !utils.isMathMLElementName(obj.id.name)) {
Copy link
Member

Choose a reason for hiding this comment

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

This rule should check only custom components.

@@ -0,0 +1,206 @@
[
Copy link
Member

Choose a reason for hiding this comment

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

We can use node.namespace to check SVG/MathML now.

Copy link
Contributor Author

Choose a reason for hiding this comment

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


## :wrench: Options

Default casing is set to `allways`
Copy link
Member

Choose a reason for hiding this comment

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

always

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@afontcu fixed

```html
<template>
<foo my-prop="prop">
<a onClick="retrun false"></a>
Copy link
Member

@afontcu afontcu Aug 1, 2017

Choose a reason for hiding this comment

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

return (same below 😇 )

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@afontcu fixed

@armano2 armano2 force-pushed the patch-15-html-attributes-casing branch from 54cd80e to 8d8e3d7 Compare August 1, 2017 17:12
@armano2 armano2 changed the title Add rule html-attributes-casing. Add rule attribute-hyphenation. Aug 1, 2017
@armano2 armano2 force-pushed the patch-15-html-attributes-casing branch from 790099f to a6342f4 Compare August 2, 2017 22:56
Copy link
Member

@mysticatea mysticatea left a comment

Choose a reason for hiding this comment

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

LGTM, thank you!

@mysticatea mysticatea merged commit 55d388c into vuejs:master Aug 3, 2017
@armano2 armano2 deleted the patch-15-html-attributes-casing branch August 3, 2017 22:02
filipalacerda pushed a commit to filipalacerda/eslint-plugin-vue that referenced this pull request Aug 5, 2017
* 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
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants