Refactor template model #263
Conversation
…s before and after the extends statement
…mplate-model Conflicts: jtwig-core/src/test/java/org/jtwig/unit/content/model/IncludeTest.java
… well as the resource loading mechanism allowing for resources to be located across multiple storage architectures
…efactor-template-model
| protected JtwigBasicParser basicParser; | ||
| protected JtwigTagPropertyParser tagPropertyParser; | ||
| protected JtwigConstantParser constantParser; | ||
|
|
There was a problem hiding this comment.
Why use protected for all variables? Any plans for them?
There was a problem hiding this comment.
There is no reason that subclasses shouldn't be able to access and/or manipulate object state. If there was a reason that object state should be immutable, then I would declare them private, but as it is conceivable that one might want to subclass the Environment for some reason, I'd rather retain the ability to easily handle the state of the subclass.
|
We need more tests on this! @thomas-p-wilson I can help you on this, just let me know what you need. |
|
Is there a chance to get some of the improvements also into the 3.x branch? Especially the caching improvements? |
|
@thomashunziker The caching changes are a pretty big departure from the way the 3.x branch works, but I'm more than happy to look into something that would remain backward compatible. |
|
@lyncodev Thanks for the offer! I'll take you up on that! I've got some minor changes coming tomorrow for caching and resource resolution, after which I'll be catching up on tests. I'll update here as I begin writing tests so that we don't write tests for the same things, sound good? |
|
@thomas-p-wilson sounds like a plan to me. |
|
|
||
| @Override | ||
| public TemplateCache addParsed(final String name, final Template template) { | ||
| parsed.put(name, template); |
There was a problem hiding this comment.
An Hashmap allows Null keys, meaning that you can have Null names on the Hashmap. Howerver, your getters don't allow Null names. You should add also a null check at put methods.
|
@thomas-p-wilson I try to be around slack if you wanna have a quick chat around the testing. |
… be closed. Also removed unused name method.
|
Nice one @thomas-p-wilson. Shall I merge it? So that we can start working on new shiny features? |
|
I'd say go for it. I/we can fix up any issues that arise as they come up.
|
Some highlights:
RenderConfiguration,CompileConfiguration, andParseConfigurationhave been combined into theEnvironmentResourceimplementations (StringJtwigResource, et al) have been refactored into a set ofLoaders andResources. TheEnvironmentis now solely responsible for managing the resolution and loading of resource. Multiple loaders may be specified through the use of theChainLoader. As a result, the resolution process should always return a path relative to an arbitrary resource root, in much the same way as we specify a resource as 'client/list.twig' and whatnot.Templateand its implementations, we now have a place to store per-template information, present examples beingMacroandBlockinformation, for later usage. This also allows us to use the SetVariable tag during template extension.