search

lark-parser / lark_cython

Cython plugin for Lark, reimplementing the LALR parser & lexer for better performance
MIT License
44 stars 5 forks source link

add __lark_meta__ dunder #10

Closed ernestoarbitrio closed 1 year ago

ernestoarbitrio commented 1 year ago

TBH I don't know how to test it better and write more meaningful test. Any suggestion is more than welcome

FIXES #9

erezsh commented 1 year ago

It's a good start, but make sure the meta attributes are filled in correctly. line, column, end_line, etc. Doesn't need to be thorough, but just to show it gets written correctly.

erezsh commented 1 year ago

Thanks.

Sorry I forgot to mention, can you also make sure to test it with a Tree instance? (you can just add a rule to the grammar, and check its meta has attributes too)

erezsh commented 1 year ago

It's odd that the tests are passing.. how can that be if I didn't merge the Lark PR yet?

ernestoarbitrio commented 1 year ago

It's odd that the tests are passing.. how can that be if I didn't merge the Lark PR yet?

I think this is not odd since I did not send the meta through the Tree. The token has the line etc... but the Tree still has meta empty. So I wonder how can I exercise a test for this scenario

ernestoarbitrio commented 1 year ago

It's odd that the tests are passing.. how can that be if I didn't merge the Lark PR yet?

maybe i got it ... writing a test

ernestoarbitrio commented 1 year ago

ok now it should fail. FYI on my machine it passes cause I have the parse_tree edited as you PR in lark

erezsh commented 1 year ago

Looks good! I'll merge it after next Lark release.

erezsh commented 1 year ago

@ernestoarbitrio The tests are still failling after releasing 1.1.4, any idea why?

ernestoarbitrio commented 1 year ago

@ernestoarbitrio The tests are still failling after releasing 1.1.4, any idea why?

looking into it

ernestoarbitrio commented 1 year ago

Now it should work

erezsh commented 1 year ago

Great, thank you for contributing!