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

Rewrite lexer to better match Vue code #2

Open
wants to merge 10 commits into
base: master
Choose a base branch
from

Conversation

lksnmnn
Copy link

@lksnmnn lksnmnn commented Oct 25, 2019

Hi there,

this PR just adds a failing test case for matching the v-for directive correctly.

Cheers,
Lukas

Test output

➜ python3 -m unittest tests/test_lexer.py
..............F
======================================================================
FAIL: test_lexing_directive_vfor (tests.test_lexer.VueLexerTestCase)
----------------------------------------------------------------------
Traceback (most recent call last):
  File "/Users/Lukas/Desktop/vue-lexer/tests/test_lexer.py", line 108, in test_lexing_directive_vfor
    (Token.Punctuation, '>')
AssertionError: Lists differ: [(Tok[55 chars]Name.Attribute, 'v'), (Token.Name.Tag, '-for')[273 chars]'>')] != [(Tok[55 chars]Name.Tag, 'v-for'), (Token.Literal.String, '="[110 chars]'>')]

First differing element 2:
(Token.Name.Attribute, 'v')
(Token.Name.Tag, 'v-for')

First list contains 7 additional elements.
First extra element 7:
(Token.Name.Attribute, 'in')

  [(Token.Punctuation, '<'),
   (Token.Name.Tag, 'path'),
-  (Token.Name.Attribute, 'v'),
-  (Token.Name.Tag, '-for'),
+  (Token.Name.Tag, 'v-for'),
?                    +

+  (Token.Literal.String, '=""link in graph.links" '),
-  (Token.Error, '='),
-  (Token.Error, '"'),
-  (Token.Name.Attribute, 'link'),
-  (Token.Name.Attribute, 'in'),
-  (Token.Name.Attribute, 'graph.links'),
-  (Token.Error, '"'),
   (Token.Name.Tag, ':key'),
   (Token.Literal.String, '="link.id"'),
-  (Token.Punctuation, '/'),
   (Token.Punctuation, '>')]

----------------------------------------------------------------------
Ran 15 tests in 0.675s

FAILED (failures=1)

@lksnmnn lksnmnn changed the title Add failing example template Add test for failing v-for template Oct 25, 2019
@lksnmnn
Copy link
Author

lksnmnn commented Oct 25, 2019

Could it be that pygments' JavaScript lexer does not know that path is a valid HTML DOM element? I don't know if this is possible, but couldn't you use the HTML lexer for the <template> part and and JavaScript lexer for the <script> part of the single file component?

Also the Travis CI setup seems to be broken.

@mjhea0
Copy link
Contributor

mjhea0 commented Oct 25, 2019

@lksnmnn

It's the periods in graph.links and link.id and the trailing forward slash at the end.

This passes just fine:

def test_lexing_directive_vfor(self):
    lexer = lexers.get_lexer_by_name('vue')
    tokens = lexer.get_tokens('''
        <path v-for="link in graph" :key="link">
    ''')

    self.assertEqual (self.__filter_tokens (tokens), [
      (Token.Punctuation, '<'),
      (Token.Name.Tag, 'path'),
      (Token.Name.Tag, 'v-for'),
      (Token.Literal.String, '="link in graph" '),
      (Token.Name.Tag, ':key'),
      (Token.Literal.String, '="link"'),
      (Token.Punctuation, '>')
    ])

Want to update the lexer to account for those?

What's wrong with the Travis config?

@lksnmnn
Copy link
Author

lksnmnn commented Oct 25, 2019 via email

@lksnmnn
Copy link
Author

lksnmnn commented Oct 25, 2019

I believe I just fixed the error for the object accessor dots. But I think the trailing slash is not as simple to fix. If it is matched, I need to '#pop' too, right?

Also I removed 3.3 from the travis config.

@mjhea0
Copy link
Contributor

mjhea0 commented Oct 25, 2019

I'd like to remove these Python versions:

  • 2.7
  • 3.3
  • 3.4
  • 3.5

Thoughts?

#3

@mjhea0
Copy link
Contributor

mjhea0 commented Oct 25, 2019

I believe I just fixed the error for the object accessor dots. But I think the trailing slash is not as simple to fix. If it is matched, I need to '#pop' too, right?

I believe so.

@mjhea0
Copy link
Contributor

mjhea0 commented Oct 25, 2019

Actually, this will capture the trailing forward slash:

(r"(<)(/?)(>)", bygroups(Punctuation, Punctuation, Punctuation)),
def test_trailing_forward_slash(self):
    lexer = lexers.get_lexer_by_name('vue')
    tokens = lexer.get_tokens('''
        <path />
    ''')

    self.assertEqual (self.__filter_tokens (tokens), [
      (Token.Punctuation, '<'),
      (Token.Name.Tag, 'path'),
      (Token.Punctuation, '/'),
      (Token.Punctuation, '>'),
    ])

I think it's probably the order of the lexers now. The .s are still not right either.

@lksnmnn
Copy link
Author

lksnmnn commented Oct 26, 2019

My fix took care of the dot (it was missing in the longest matcher for a :key attribute. I addded 2 more tests, one with dots but without the self closing tag. And the empty self closing tag you wrote above.

I played a little bit around with the regex and it seems to me that the issue is, that all the special v-xyz matchers must match a (>) to work. But I think this is not the way a lexer should work, as this character is matched by another punctuation matcher. When I remove this last group every test works just fine. Could you confirm this? (I only removed it from the longest regex with v-for and :key)

Edit: When I remove this from all of the matchers, some other tests fail to match strings in between tags (like: test . the tester expects "test" to be an attribute (which is not correct imo? should be matched to Text, compare: https://github.com/dagwieers/pygments/blob/master/pygments/lexers/html.py) So there is a deeper issue here.

Edit2: I think I got it now. I think the right way to this, would be to just extend tag and attribute matchers to allow for v-words, @s and colons to match as attributes. And whatever special syntax vue brings. I think this would slim down the current token list a lot.

For now I can highlight my code with this fix, but after I am done with my thesis, I may dive back into this and fix it for good :)

Edit3: I got started doing this, but I am stuck now, matching the inner HTML of a tag to text, like so:
<span>text</span>. This is due to the fact, that the basis here is a javascript lexer and not an HTML lexer. So the cleanest thing to do is to use the HTML lexer for within render and <template> and use the script handler for <script>.

@lksnmnn
Copy link
Author

lksnmnn commented Oct 26, 2019

Hm. One weird thing you maybe could help me with is, that I installed my updated version, but pygmentize seems to use the old version somehow.

When I remove it from site-packages I do get an error because of a missing lexer for .vue files.
I now installed it as "develop" pip3 install -e path/to/vue-lexer but still, the highlighted output is the same.

When I add my test file to the unit tests, it tokenizes the code correctly!
Also this works (using stdin):

pygmentize -O full -f html -o test.html -l ~/Desktop/vue-lexer/vue/lexer.py:VueLexer -x
<template>
    <svg viewBox="-200, -200, 400, 400">
        <g id="links">
            <path 
                v-for="link in graph.links" 
                :key="link.id" 
                :d="`M ${link.source.x},${link.source.y} ${link.target.x},${link.target.y}`"
            />
        </g>
        <g id="nodes">
            <circle 
                v-for="node in graph.nodes"
                :key="node.id" 
                :cx="node.x" 
                :cy="node.y" 
                :r="getNodeRadius(node)"/>
        </g>
    </svg>
</template>

Edit: I think this change is on a good way. What we need to add is different highlighting for normal attributes (without a directive or binding) and I'd wished for nicer expression highlighting within directives or bindings. If we want it super clean, the above mentionend separation between template, render and script should be made imho.
It now looks like this for my template:

Bildschirmfoto 2019-10-26 um 13 08 46

@mjhea0
Copy link
Contributor

mjhea0 commented Oct 28, 2019

Just getting caught up on this. I'm a little confused on what else needs to be done.

@lksnmnn
Copy link
Author

lksnmnn commented Oct 28, 2019

I will did push the code, which is working for me right now. But I haven't fixed all failing tests yet and for some cases, the lexer needs to be extended even more, I think.

Maybe you can have a look and let me know if this going in the right direction :)
The main changes are:

  • Do not attempt to match a complete DOM tag
  • Instead, extend the existing matchers to allow tags with hyphens (<vue-x-component>)
  • match directives and bindings as attributes (v-for, :key, @click)
  • match = as operator
  • match "" as javascript expression

@lksnmnn lksnmnn changed the title Add test for failing v-for template Rewrite Lexer to Match Vue Code Correctly Oct 28, 2019
@lksnmnn lksnmnn changed the title Rewrite Lexer to Match Vue Code Correctly Rewrite lexer to better match Vue code Oct 28, 2019
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.

None yet

2 participants