-
Notifications
You must be signed in to change notification settings - Fork 285
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
Fixed whitespace handling for partials #212
Conversation
This seems reasonable. I may not want to make it the default behavior as it isn't backwards compatible with the current whitespace handling but I'm open to that conversation as well. Thanks. |
Took a look at the code. I think that this could be better done during compilation rather than at runtime. The current version will definitely affect performance by a lot. Since we know the indentation at parse time we could probably modify the text found in the partial at the same time to adjust the indentation. The only odd situation would be recursion... |
I also thought about the performance problem. From what I see, for non-recursive templates we could do this at compilation but will create a larger cache / compiled data structure as we would need a different partial object for each indentation. E.g. if a partial is used at 4 different indentation levels we would need to create 4 different variants of it. For recursive templates I don't see a way around doing this at runtime. An example why this could be useful even for recursive templates:
Called with
should result in
I could not find a spec case for indentation and whitespace. However the ruby implementation which seems to be quite "official" actually yields the expected outcome. So the question is if it is worth the extra complication and memory footprint to have a compile-time solution for non-recursive partials. One could have a switch that enables whitespace-conformity, allowing for faster rendering if whitespace is not an issue or slower rendering if it is. For backwards compatibility the switch could be off by default. If needed, the switch could even be implemented at compile time, by using different variants of |
I've known about the whitespace issue for a long time, it hasn't been implemented because of the performance issues with it and almost all of the generated text isn't sensitive to it (HTML). Let me see if there is a way to implement it with some overrides so that if people want this behavior I could include the ability to do it without changing the normal high performance behavior. |
I took the time to update my changes so that by default nothing changes and performance is not impacted. For people that want whitespace according to the specs I added another To make review easier, I handle only the (most problematic) case of partials in this PR. See #215 for a solution that fixes whitespace for all spec cases. |
I'd like to see this merged and published. Using the SpecMustacheFactory seems like a nice opt-in solution here for those of us using mustache for whitespace significant output formats. |
Will this get merged? |
We'd need it too, generating yaml spec with mustache, where indentation counts. |
I assume this still hasn't been released yet? I have a mustache library (https://github.com/jstachio/jstachio) that so far has achieved mostly 1:1 with JMustache as both my library and JMustache implement the whitespace but I cannot with mustache.java because of the whitespace issue (as well as lambda #286 ). Ideally I was hoping for duplicate output to reuse tests as well as make an option for people using JStachio to safely switch to mustache.java if runtime templates are needed. |
As mentioned in #211 the handling of whitespace for partials of mustache.java is not conforming to the mustache spec.
This PR fixes this (only for partials though).
Basic idea is to memorize the indentation of a partial and to pass it through during execution via a custom writer.
This is not yet finished, as this obviously broke some other tests (as the whitespace differs). I just wanted to know if this is at all a direction that would be considered for merging.