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鈥檒l occasionally send you account related emails.

Already on GitHub? Sign in to your account

WIP chore: drop twing for twig-lexer #17

Open
wants to merge 3 commits into
base: master
from

Conversation

Projects
None yet
2 participants
@Kocal
Copy link
Owner

commented Jul 1, 2019

@ericmorand have created the perfect package to lex Twig code: twig-lexer

When using Twing, we had some issues about whitespace control modifier ({%-, {{-, ...) because they were simply removed from tokens and nodes (see ericmorand/twing/issues/321 and ericmorand/twing/pull/326).

But with twig-lexer everything is preserved as tokens, example:
S茅lection_916
And so it means that we will be able to print those modifiers back :)

Actually I will have to rewrite everything since we will use tokens instead of nodes to prettify Twig code.聽In fact not everything, but we have to make sure we are using an AST and not only tokens (even with a token stream), otherwise it will be a pain in the ass.
Since we won't directly use tokens for an AST, we need to find a way to reserve whitespace control modifiers. That can be weird, but I think we can store (on certain types of node) the opening/closing tags with whitespace modifier controls. 馃

And also use FastPath instead of our own print function because of (prettier/plugin-php#12):

  • comment handling around every single node
  • access to the parents all the way to the root
  • It also allows the multiparser feature to exist

This PR will close #15, close #4 and close #13

Kocal added some commits Jun 27, 2019

@ericmorand

This comment has been minimized.

Copy link

commented Jul 5, 2019

Glad to see that this will help you. It will be the lexer used by the next major version of Twing.

Just a remark about the "WHITESPACE" tokens. They may seem redundant - after all they can easily be resolved by simple comparison of value and line/column properties of the tokens - but I felt they would make life easier for tools like yours - less code from your side is always better.

The drawback is that they have to be ignored by eventual parsers (like Twing parser) but this is trivial.

Another remark : I see that you implemented a token stream which is very good. I have been torned between having one included in twig-lexer or not. Actually, Twing will also need one so maybe it could be beneficial to have it included in twig-lexer and tested there instead of having it spread in multiple projects. It would come as a separate module that export a constructor that takes an array of tokens as parameter. With tree shaking, it would add zero byte to consumer packages if not used.

@Kocal

This comment has been minimized.

Copy link
Owner Author

commented Jul 5, 2019

Don't worry for the WHITESPACE tokens, in fact I think I will skip everyone of them since it's Prettier that will decide how to render whitespaces (see https://github.com/Kocal/prettier-plugin-twig/blob/master/src/printer.js#L59 and https://github.com/prettier/prettier/blob/master/commands.md#line).

For the token stream, it would be really nice since the one I wrote is like 80% copy/pasted from Twing. 馃槄

@ericmorand

This comment has been minimized.

Copy link

commented Jul 6, 2019

It's a deal. I'll add it to twig-lexer. And loot your test suite if you don't mind.

@Kocal

This comment has been minimized.

Copy link
Owner Author

commented Jul 6, 2019

Yeah go for it! :)

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can鈥檛 perform that action at this time.