WIP 🚧: Support Whitespace Control For all Tags and Objects #67
Conversation
Jcambass
force-pushed
the
whitespace_support_for_tags
branch
10 times, most recently
from
d9bc98b
to
974e81a
Compare
|
Thanks for starting this! I think it's the hardest part of rendering Liquid templates! Happy to help with anything I can |
Jcambass
force-pushed
the
whitespace_support_for_tags
branch
3 times, most recently
from
7e61313
to
1ef605e
Compare
|
@edgurgel Thanks for offering your help! I will now first try to add test cases for every tag with (hopefully) all whitespace situations. I hope that's ok with you. I though about generating those cases but I think it's clearer to understand with the explicit approach. I will add some checkboxes for stuff that is working/not working once all test cases are here :) |
Jcambass
force-pushed
the
whitespace_support_for_tags
branch
7 times, most recently
from
23bb4ea
to
b0d9ce1
Compare
Jcambass
force-pushed
the
whitespace_support_for_tags
branch
from
b0d9ce1
to
5f33dfb
Compare
While implementing [whitespace support](edgurgel#67) I came to the conclusion that it makes sense to auto generate different whitespace trimming variants of a given template to gain a good test coverage of edge cases while keeping test maintainable. One problem I encountered, was that since the templates where generated I didn't know the actual input that was used when comparing the render results of liquid and solid. My solution was adding a new `assert_render` macro that renders the template in liquid and solid and then compares them (as we do already with `cases_test`), but then includes the passed template in the assertion output if there is an missmatch. I'm trying to keep the main patch for the whitespace support as small and revieable as possible. Since this particular change can stand alone and IMHO makes also sense outside of the context of whitespace support I've opened this PR. Sample output: ``` 1) test case 113 (:"Elixir.Solid.Integration.Cases.113Test") test/integration/cases_test.exs:17 Render result was different! Input: {% if enabled? %} ON {% else %} OFF {% endif %} code: liquid_output == solid_output left: "\nON\n\n" right: "\nOFF\n\n" stacktrace: test/integration/cases_test.exs:21: (test) ```
While implementing [whitespace support](edgurgel#67) I came to the conclusion that it makes sense to auto generate different whitespace trimming variants of a given template to gain a good test coverage of edge cases while keeping test maintainable. One problem I encountered, was that since the templates where generated I didn't know the actual input that was used when comparing the render results of liquid and solid. My solution was adding a new `assert_render` macro that renders the template in liquid and solid and then compares them (as we do already with `cases_test`), but then includes the passed template in the assertion output if there is an missmatch. I'm trying to keep the main patch for the whitespace support as small and revieable as possible. Since this particular change can stand alone and IMHO makes also sense outside of the context of whitespace support I've opened this PR. Sample output: ``` 1) test case 113 (:"Elixir.Solid.Integration.Cases.113Test") test/integration/cases_test.exs:17 Render result was different! Input: {% if enabled? %} ON {% else %} OFF {% endif %} code: liquid_output == solid_output left: "\nON\n\n" right: "\nOFF\n\n" stacktrace: test/integration/cases_test.exs:21: (test) ```
|
I currently hit a wall with this PR. |
|
@Jcambass thanks for the heads-up. I will have a look at the PR this weekend and see if I can help somehow. |
|
@edgurgel Sounds good. Be aware that it lacks some proper structure and direction. I was switching the place where trimming happens multiple times up and down the layers but didn't find the right spot. The liquid gem parses in a way where it operates more directly on the whole tag instead of first parsing it into clear datastructures like we do with nimbleparsec. It feels like there is some fancy term like small step semantics vs big steps semantics that might describe the difference but I'm not fully sure. I have a hard time mapping the solution liquid applies to a solution that could work for solid while keeping the exact semantics without adding hardcased edge cases. So before you get lost in this PR it might be better to have a look at liquids whitespace handling and think about how it conceptionally would transition to solid. |
Jcambass
force-pushed
the
whitespace_support_for_tags
branch
from
5f33dfb
to
ed55d22
Compare
|
@Jcambass Here's an idea... what if we "annotate" the places where we must trim whitespaces and then on a second pass (after everything has been parsed and rendered we go through the whole rendered binary to trim the whitespaces? Parse -> render -> trim whitespaces ? It's not super efficient but at it seems more feasible and at least the feature gets implemented? We could then in the future try to optimise it. I haven't tried doing this as a separate step but I wonder if it would work 🤔 |
|
Closed in favor of #84. |
This patch is still very much WIP and a bit exploration but I opened it as a draft for visability and maybe gathering some needed inputs.
Also don't try to understand the random comments, that's just me trying to note my thoughts (which are very likely nonsense in many cases but I still want them to verify before just ignoring them :) ).
The major problems at the moment are:
See: #10