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

Generic Processors #1045

Merged
merged 6 commits into from
Apr 9, 2023
Merged

Generic Processors #1045

merged 6 commits into from
Apr 9, 2023

Conversation

boulter
Copy link
Contributor

@boulter boulter commented Apr 7, 2023

I decided to just implement a more generic version of #1044 so we could easily add processors for pre and post execution of nodes and now filters. In the future we could add tags and functions to the builder.

@boulter boulter requested review from jasmith-hs and hs-lsong April 7, 2023 13:43
@boulter boulter mentioned this pull request Apr 7, 2023
Copy link
Contributor

@jasmith-hs jasmith-hs left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I like the idea of making this usable across more operations in Jinjava. Are there any filters, in particular, you're interested in timing?

Comment on lines 71 to 82
JinjavaProcessors processors = interpreter.getConfig().getProcessors();

try {
if (processors != null && processors.getFilterPreProcessor() != null) {
processors.getFilterPreProcessor().accept(this, interpreter);
}
return filter(var, interpreter, filterArgs);
} finally {
if (processors != null && processors.getFilterPostProcessor() != null) {
processors.getFilterPostProcessor().accept(this, interpreter);
}
}
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Many filters override this. Also, this doesn't time filtering when the var is a SafeString
If you really want to time all filters, you'd probably need to proxy the classes in filters, exptests, and functions in FilterLibrary, ExpTestLibrary, and FunctionLibrary when calling addFilter, addExpTest, and addFunction, respectively.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

yeah, I know it won't catch everything, but it does catch most. I added a comment to explain that.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

You could instead add processors within JinjavaInterpreterResolver.invoke or some other place within the ELResolver code to get around this.

@boulter
Copy link
Contributor Author

boulter commented Apr 7, 2023

I like the idea of making this usable across more operations in Jinjava. Are there any filters, in particular, you're interested in timing?

I'm not really concerned with filters right now, but it was an easy demonstration for how to use processors for functionality besides nodes. Nodes probably covers it for what I'm concerned with since all filters and functions will be part of a node.

@boulter boulter requested a review from jasmith-hs April 7, 2023 15:48
@boulter
Copy link
Contributor Author

boulter commented Apr 9, 2023

I've removed the filter processors for now since I don't need them.

@boulter boulter merged commit f80ca19 into master Apr 9, 2023
@boulter boulter deleted the processors branch April 9, 2023 00:14
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.

3 participants