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

Implement ChunkResolver by extending ExtendedParser #527

Closed
wants to merge 14 commits into from

Conversation

jasmith-hs
Copy link
Contributor

@jasmith-hs jasmith-hs commented Nov 6, 2020

@jboulter Because of your comment about the parser that exists in Jinjava, I decided to look into it as an alternative to the ChunkResolver PR that I opened #525.
It essentially does the same thing that the ChunkResolver does but in a more low-level (and probably faster) way.

Rather than introducing a new parser that works on top of the interpreter.resolveELExpression(), this approach instead extends the Jinjava parser itself by providing decorators for all the different types of AstNodes to allow for deferred values to be handled when they are evaluated.

I created a new Exception type that passes through the Abstract Syntax Tree as it gets evaluated, and when one of the decorated nodes sees this exception, it partially evaluates its children (or whatever they are specifically for each type of AstNode).

This method for partially resolving expressions is more complex than the initial ChunkResolver PR because I had to implement custom overrides for all the different AstNodes.

This is a draft PR because idk if I want to go with this approach. It's a lot more complex than the ChunkResolver, and I haven't fully tested out all the code (I only have it tested against the tests that I wrote for the ChunkResolver PR). A pro to this approach is that I think it is faster than the other PR because the expression gets evaluated a single time, rather than being evaluated several times in sub-expressions.

I don't intend for anyone to read through this code, it's currently just a proof of concept, unless we decide to go with this approach.

@boulter
Copy link
Contributor

boulter commented Nov 9, 2020

Thanks for trying this out. It's a bummer that it would require overriding all those classes. I don't think performance is a big concern here so I guess the original approach makes more sense.

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.

2 participants