slime-lang / slime

Minimalistic HTML templates for Elixir, inspired by Slim.
http://slime-lang.com
MIT License
368 stars 57 forks source link

Strip windows line endings #137

Closed tmbb closed 7 years ago

tmbb commented 7 years ago

All credit goes to sztosz, who fixed this issue originally. Added test.

Fixes https://github.com/slime-lang/slime/issues/107

Rakoth commented 7 years ago

Hi, @tmbb could you please provide more details about the issue, parser now should handle windows line ending, we have corresponding test case:

https://github.com/slime-lang/slime/blob/master/test/renderer_test.exs#L91-L93

  test "CRLF line endings are converted to LF" do
    assert render("h1\r\n\th2\r\n\t\th3 Hi\r\n") == "<h1><h2><h3>Hi</h3></h2></h1>"
  end
tmbb commented 7 years ago

That's strange... I admit I didn't read the whole source, but the master failed in my machine before my fix and started working with my fix. I'll try to figure it out.

tmbb commented 7 years ago

So far:

== Compilation error on file lib/talon_blog_demo/talon/views/admin/admin-lte/layout_view.ex ==
** (Slime.TemplateSyntaxError) got FunctionClauseError with message "no function clause matching in String.duplicate/2" while retrieving Exception.message/1 for %Slime.TemplateSyntaxError{column: 0, line: "  head\r", line_number: 3, message: "{:no_match, <<14>>}", source: "INPUT"}
    (slime) lib/slime/parser.ex:27: Slime.Parser.handle_syntax_error/3
    (slime) lib/slime/renderer.ex:17: Slime.Renderer.precompile/1
    (phoenix_slime) lib/phoenix_slime/engine.ex:11: PhoenixSlime.Engine.compile/2
    (phoenix) lib/phoenix/template.ex:378: Phoenix.Template.compile/2
    (phoenix) lib/phoenix/template.ex:186: Phoenix.Template."-MACRO-__before_compile__/2-fun-0-"/3
    (elixir) lib/enum.ex:1755: Enum."-reduce/3-lists^foldl/2-0-"/3
    (phoenix) expanding macro: Phoenix.Template.__before_compile__/1
    lib/talon_blog_demo/talon/views/admin/admin-lte/layout_view.ex:1: TalonBlogDemo.Admin.AdminLte.Web.LayoutView (module)
    (elixir) lib/kernel/parallel_compiler.ex:117: anonymous fn/4 in Kernel.ParallelCompiler.spawn_compilers/1

Not that the final error comes from trying to call String.duplicate(" ", -1), as the error reported at column 0. This is strange (the code doesn't seem to think that an exception at column 0 is possible), but that's not the most important part. The most important part is the syntax error.

I'm not yet deep enough into Slime's source, but the error message ("{:no_match, <<14>>}") seems to indicate an unexpected "\r" character.

The repo of the project is this: https://github.com/talonframework/talon_blog_demo

Steps to repeat (on windows):


The problem goes away with my fix. My fix works at the level of the preprocessor. I don't know what's the relevance of that fact...

The problem also goes away with if {:slime, "~> 0.16.0"} is used instead of 1.0.0.

Thanks for taking the time to work on this issue.

Rakoth commented 7 years ago

I see, the problem is with preprocessor, which inserts virtual symbols of indent <<14>> and dedent <<15>>, but since preprocessor splits lines by \n it inserts one of them between \r and \n. Could you narrow it down to exact slime template, which causes this problem, so we can add a test case?

tmbb commented 7 years ago

I haven't done any formal analysis to try to find out which templates break Slime on windows, but I've managed to shrink my test case into a possibly minimal example.

It would be interesting to add property testing to Slime. It would have caught this bug in testing (note, that you already had a test case for CRLF problems, but only a slightly more complex template found the error; such more complex templates could be generated as a matter of by property testing frameworks).

If I find the time to learn something like propcheck (the custom element generators seem great), I can submit a pull request.

Rakoth commented 7 years ago

Ok, I see the problem now, it is with empty line in template. Sure @tmbb you can send a PR with propcheck 👍