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

Replace Slime.Parser with parser generated from PEG (using neotoma) #113

Merged
merged 11 commits into from
May 9, 2017

Conversation

Rakoth
Copy link
Member

@Rakoth Rakoth commented Nov 13, 2016

This is experimental implementation of a parser using PEG

First thing to look at is grammar src/slime_parser.peg
Then take a look at new preprocessor lib/slime/parser/preprocessor.ex which main goal is to add indents/dedents where needed (I get the ideas from here)
Finally take a look at transform callback module lib/slime/parser/transform.ex This is where parsed AST gets transformed into useful one. Its current goal is to produce tokens exactly as our current parser do. But it can be easily modified to produce nested structures as Slime.Tree do.

Things to discuss:

  • Performance issue.
    The bummer is that this parser is ~10x slower than our current parser. But I believe grammar might be optimized to reduce the performance gap. See benchmark bench/example_parse_bench.exs.
  • Handle embedded engine code with inconsistent indents.
    There is currently a pending test case about this issue. The possible solution is to make preprocessor smart enough to skip embedded engine lines and do not add indents/dedents.
  • Make :attr_list_delims work again.
    There is penging test case
    The possible solutions are:
    • Generate slime_parser.peg file from a template and add only supported delimiters into grammar
    • Pass original iolist with transformed AST through and in transform(:wrapped_attributes, ...) based on config options chose ether transformed attributes or the original input.
    • Drop support :(
  • Raise an error on unmatched attrs delimiters.
    (When there is a space separating tag head and wrapped attributes list). Pending test. While it is currently supported and is supported by original slim, I found it hard to manage using PEG, such attributes now parsed as inline text after wrapped attributes check is failed.
  • Grammar and transform can by simplified.
    • if we remove empty lines in preprocessor, but then we need to map errors to original input lines.
    • other suggestions?

Things to do next:

  • Since preprocessor now adds symbols to original input, it is necessary to map errors position to original input. And provide useful error messages on unexpected indents/dedents.
  • Parse nested tags structure, remove Slime.Tree
  • PR to neotoma project to support Elixir module as transform_module option?
  • Separate static code and dynamic elixir insertions in strings with interpolation
  • Compile to function omitting eex representation (great deal is done by @lpil here)

Closes #106 #32 #123

@doomspork, @henrik, @lpil, @bozydar I need your thoughts on this, guys.

@doomspork
Copy link
Member

Thank you for this @Rakoth, this is awesome! Sorry for the delay getting back to you, I've been swapped with work and family things.

Could we sync up on Slack and you give me a rough run down of things? This is a new approach for me.

@doomspork
Copy link
Member

@Rakoth once the merge conflicts are resolved is this safe to merge? Will we retain the same level of functionality we currently have?

@doomspork doomspork merged commit 60d6a68 into slime-lang:master May 9, 2017
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants