-
-
Notifications
You must be signed in to change notification settings - Fork 75
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
Custom template tokenizers #148
Conversation
Thank you for this PR. |
Also, can you tell me why it's better than eslint-plugin-vue-pug-sfc when it works with Pug? |
By the way, I don't use Pug, so I'm not familiar with Pug. |
That's great to hear!
I'll add test cases for sure. The I'll document how the interface will look, it's something like this 9720b51#diff-3bb9c55886cd92e12d33790e215997dbd9a7e91b339459b52726f51ce9b0a017R6 . The custom template tokenizer receives the raw text string from inside the
Mainly for the same reason I opted for emitting tokens instead of a full AST in this PR. Implementing all the functionality vue-eslint-parser offers would mean reimplementing a lot of code in eslint-plugin-vue-pug-sfc, especially everything regarding parsing js expressions in the template. eslint-plugin-vue-pug-sfc could actually use this custom tokenizer to implement rules specific to pug without the need to rebuild every rule from eslint-plugin-vue. I only tested the I will continue working on this PR and adding tests. |
I plan to make breaking changes to the |
I will merge #144 😅 |
@rashfael @ota-meshi ping me if you need a review or maybe answers for some questions if there are some. |
I have documented the interface and wrote a test with a mock tokenizer. which just outputs a hardcoded set of tokens. The mock tokenizer gets some locations wrong, but I think for testing purposes this does not matter much. I will still try to fix the wrong locations. I think I have accounted for all possible features. I will continue writing the actual pug tokenizer and testing eslint-plugin-vue rules which may reveal something I missed. @Shinigami92, a review of my changes and feedback would be much appreciated. |
I had a look over code and docs, don't fully understand yet everything due to I didn't worked with it in action, but I also currently do not have the time to go so much in depth, so I trust your code. |
Just a quick status update: I find it unlikely that I will find any problem with the API implemented in this PR. @Shinigami92 do you want to discuss how we want to proceed with pug linting in general? |
Uhm yeah, we could do this in a GitHub discussion somewhere 🤔 |
I have continued my work at eslint-plugin-vue-pug and all base and essential rules are running out of the box, only one rule needed a replacement. I'll continue with testing all existing I also tested the whole setup on a large real world project of mine and linting is working well. @ota-meshi are you happy with this PR? |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I don't have time to look at this PR right now. I found one, so I will comment.
Could you change the |
Having the default behaviour ignore custom languages like pug kinda defeats the purpose of being able to just re-use existing rules. As it is right now, over 90% of the rules from |
Additionally, this PR won't change any behaviour as long as you don't configure a |
Do you say that eslint-plugin-vue will remain officially unsupported for Pug?
I thought that only the eslint-plugin-vue rule that added the Pug test (and use visit option of defineTemplateBodyVisitor) should work with Pug. If you don't need to officially support Pug, I think the method as you said is ok. |
Yes, I was thinking eslint-plugin-vue and vue-eslint-parser do not need to have any specific pug code and don't need to support it.
Sounds good, I'll add a disclaimer that this setting is intended for plugin developers and if you are looking for pug support, you should use eslint-plugin-vue-pug.
In vue-eslint-parser I only added a test to check if the custom tokenizer abstraction is working, in eslint-plugin-vue-pug I am testing and tracking every eslint-plugin-vue rule if they work with concrete pug templates. |
I also communicated with @rashfael, and when eslint-plugin-vue-pug works, I will archive my |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I made only one comment on the source code.
I think we should add more notes to the document. I think that the specification of the templateTokenizer
option will change even in the minor version, so I think it is good to mark it as an experimental feature.
…instead of maintaining a flag
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM
This PR adds support for custom template tokenizers and can be used to add pug support externally.
Custom template tokenizers can be configured in
parserOptions
:The custom tokenizer generates
IntermediateTokens
like the ones generated by theIntermediateTokenizer
, which then get consumed by the html parser. I think this is the easiest way to not have the custom tokenizer worry about attribute/directive parsing.There is still some functionality missing but what do you think about adding pug support in this way?
TODOs:
tokens
errors
comments
with ones from the custom tokenizer.tokenizer.expressionEnabled
Refs #29
I started an implementation for a pug template tokenizer here: https://github.com/rashfael/vue-eslint-parser-template-tokenizer-pug