Skip to content

Add "no-multi-spaces" rule (fixes #133) #138

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

Merged
merged 2 commits into from
Aug 14, 2017

Conversation

armano2
Copy link
Contributor

@armano2 armano2 commented Aug 6, 2017

This PR implement rule proposed in #133

@armano2 armano2 changed the title Init rule & add basic tests WIP: Rule Proposal: vue/no-extra-whitespaces Aug 6, 2017
@armano2
Copy link
Contributor Author

armano2 commented Aug 6, 2017

@mysticatea is there a way that we can have access to VAttributeText or something similar with whitespaces between attributes? or we can just add to VAttribute all spaces before.

for now i had to parse entire VStartTag to remove unnecessary spaces.

This is going to be needed for html-indent rule.

@armano2 armano2 changed the title WIP: Rule Proposal: vue/no-extra-whitespaces WIP: Add "no-extra-space-between-attributes" rule (fixes #133) Aug 6, 2017
@mysticatea
Copy link
Member

mysticatea commented Aug 7, 2017

I think that this rule should be token-based.

return {
  Program(node) {
    // TODO: Check `context.parserServices.getTemplateBodyTokenStore` exists or not.
    const tokenStore = context.parserServices.getTemplateBodyTokenStore()
    const tokens = tokenStore.getTokens(node.templateBody, {includeComments: true})

    let prevToken = tokens.shift()
    for (const token of tokens) {
      const onSameLine = (prevToken.loc.end.line === token.loc.start.line)
      const spaces = token.loc.start.column - prevToken.loc.end.column
      if (onSameLine && spaces >= 2) {
        // Report.
      }
      prevToken = token
    }
  }
}

@mysticatea
Copy link
Member

Spaces which don't change semantic are on outside of tokens, so we can check only extra spaces in this way.

@armano2
Copy link
Contributor Author

armano2 commented Aug 7, 2017

@mysticatea thank you for suggestion, i'm still learning how to work with eslint rules 😄

@armano2
Copy link
Contributor Author

armano2 commented Aug 7, 2017

@mysticatea i took your sugestion and i applied it to my code also i added basic doc for it.

i think we should change error message: Extra whitespace detected. seems bad 🍶

@armano2 armano2 changed the title WIP: Add "no-extra-space-between-attributes" rule (fixes #133) WIP: Add "no-multi-spaces" rule (fixes #133) Aug 7, 2017
@armano2
Copy link
Contributor Author

armano2 commented Aug 7, 2017

i consider <tag div=""/> and <tag div="" /> as valid but only when there is non or one white-space before /

we can add parameter to for this but i'm not sure if this should be in this rule

@armano2 armano2 changed the title WIP: Add "no-multi-spaces" rule (fixes #133) Add "no-multi-spaces" rule (fixes #133) Aug 7, 2017
@armano2 armano2 force-pushed the no-extra-space-between-attributes branch from 4185212 to ce8f6d1 Compare August 7, 2017 18:03
@mysticatea
Copy link
Member

@armano2 I think better if this rule focuses on only the sequences of multiple spaces. In core rule case, no-multi-spaces is doing that, and for example, space-before-function-parentheses focuses on one or zero spaces which are followed by the parenthesis of function parameters.

@@ -28,6 +28,7 @@ function create (context) {
return
}

// TODO: Check `context.parserServices.getTemplateBodyTokenStore` exists or not.
Copy link
Member

Choose a reason for hiding this comment

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

This is inside of registerTemplateBodyVisitor, so context.parserServices.getTemplateBodyTokenStore should always exist.


let prevToken = tokens.shift()
for (const token of tokens) {
if (sourceCode.isSpaceBetweenTokens(prevToken, token)) {
Copy link
Member

@mysticatea mysticatea Aug 8, 2017

Choose a reason for hiding this comment

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

This method cannot be used here because this method has special behavior that it ignores /* */ style comments which are not comments in HTML context.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

ok

type: 'HTMLIdentifier'
}
]
}
Copy link
Member

Choose a reason for hiding this comment

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

I'd like to see tests for the expression of Vue.js directives and mustaches. Those are parsed by script parser (espree by default), so those have valid tokens. This rule can report on multiple spaces which are in the expressions.

@mysticatea
Copy link
Member

i think we should change error message: Extra whitespace detected. seems bad 🍶

For reference, this is the message of core no-multi-spaces rule.

}
},
message: 'Extra whitespace detected.',
fix: (fixer) => fixer.removeRange([prevToken.range[1] + requiredSpaces, prevToken.range[1] + spaces])
Copy link
Member

Choose a reason for hiding this comment

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

This can be simpler a bit. It's "put a space between the previous token and the current token". Original spaces may be \t.

-                fix: (fixer) => fixer.removeRange([prevToken.range[1] + requiredSpaces, prevToken.range[1] + spaces])
+                fix: (fixer) => fixer.replaceTextRange([prevToken.range[1], token.range[0]], ' ')

Copy link
Contributor Author

Choose a reason for hiding this comment

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

in this case:

fix: (fixer) => fixer.replaceTextRange([prevToken.range[1], prevToken.range[1] + spaces], ' '),

is going to be valid cus there can be \n\r symbold before space

@mysticatea
Copy link
Member

mysticatea commented Aug 8, 2017

@armano2 Please use [email protected] since I found a bug about token's range (vuejs/vue-eslint-parser@d670ac9). That version fixed a bug that = tokens include following spaces.

@armano2
Copy link
Contributor Author

armano2 commented Aug 8, 2017

@mysticatea thank you for suggestions, i applied most of them except that i changed a little fixer:

fix: (fixer) => fixer.replaceTextRange(
  [prevToken.range[1], prevToken.range[1] + spaces],
  requiredSpaces ? ' ' : ''
)

@mysticatea
Copy link
Member

In core, no-multi-spaces doesn't warn trailing spaces. It's covered by no-trailing-spaces. I think it makes sense.

@armano2
Copy link
Contributor Author

armano2 commented Aug 8, 2017

@mysticatea than its ok now?

@mysticatea
Copy link
Member

mysticatea commented Aug 8, 2017

I think better if this rule focuses on only the sequences of multiple spaces between tokens. Could you remove the following things?:

  • The special handling of spaces which are before >. This should be covered by an independent rule with always/never options. Maybe space-before-tag-close or something like.
  • Trailing spaces. This is covered by no-trailing-spaces.

@armano2
Copy link
Contributor Author

armano2 commented Aug 8, 2017

@mysticatea ok changes applied


first time i didn't understand you

@armano2
Copy link
Contributor Author

armano2 commented Aug 8, 2017

also idk if this is bug in parser but HTMLSelfClosingTagClose and HTMLTagClose has no value.

i like space-before-tag-close, i can work on it tomorrow.

@armano2 armano2 force-pushed the no-extra-space-between-attributes branch from bb20abd to 876fabf Compare August 8, 2017 22:23
@armano2 armano2 force-pushed the no-extra-space-between-attributes branch from 876fabf to 014110b Compare August 8, 2017 22:25
@mysticatea
Copy link
Member

LGTM, thank you for the great work, @armano2 !

also idk if this is bug in parser but HTMLSelfClosingTagClose and HTMLTagClose has no value.

It's intentional. Those kinds of tokens don't have variation, so their value property is empty.

case 'HTMLTagClose': return '>'
}

return token.value
Copy link
Member

Choose a reason for hiding this comment

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

Ah, also the value property of HTMLAssociation tokens is empty.
I think context.getSourceCode().getText(token) is good for this purpose.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

thank you i learned few new useful thinks while building this rule

@armano2
Copy link
Contributor Author

armano2 commented Aug 9, 2017

@mysticatea requested changes applied

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.

Thank you!

@mysticatea mysticatea merged commit 140a276 into vuejs:master Aug 14, 2017
@armano2 armano2 deleted the no-extra-space-between-attributes branch August 14, 2017 08:47
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.

2 participants