search

reshape / sugarml

a whitespace-significant html parser for reshape
Other
37 stars 8 forks source link

Fix parser error #59

Closed Thomas-git closed 5 years ago

Thomas-git commented 5 years ago

Hi,

Text after a tag prevent parser from seeing nested tag on next line. Here is a patch + test case.

Best regards, Thomas

jescalan commented 5 years ago

Is there any issue with a fix like this?

div
  | foo
  span bar
Thomas-git commented 5 years ago

This bug can be avoided when rewriting .sgr as you said. Your right.

By the way my fix introduced an ambiguity. Please look at the files changes.

Thomas-git commented 5 years ago

My fix add another bug : try adding "li: a.foo hello" to inline.sgr and "<li><a class="foo">hello</a></li>" to inline.html I think it should be possible to do

div foo
  span bar

and have expected result. if you agree I will spend time fixing everything.

Regards, Thomas

jescalan commented 5 years ago

I'm not sure I agree to be honest. The same goal can be achieved with my syntax example above, and it's more clear that way as well.

We do have an inline tag bug though which is documented here, which I'd love to see a fix for! https://github.com/reshape/sugarml/issues/32

Thomas-git commented 5 years ago

Ok, this commit fix introduced bugs and bug #32 you told me about. However, as we talked about previously, parsing of

li:a bar
  span foo

and

li bar
  span foo

changed.

The former is now forbidden syntax where previously span was li child. The last produce a span child of li. Where previously span was sibling of li (not honoring the indent).

So we probably need a version bump as it will break some templates. Of course your free to keep only the changes you like.

Oh, and sorry for my ; at and of lines... A bad habit from other languages !

jescalan commented 5 years ago

Hey so I really appreciate your contribution here, but to be entirely honest I don't feel like I understand why this syntax addition is necessary. There are no other whitespace-dependent html syntaxes that I'm aware of that support this behavior because of the ambiguity it adds, and the fact that the desired functionality can be easily achieved using a pipe, as shown in my example earlier. As far as I can see, adding it is simply a personal preference, not an upgrade in functionality - as such, it doesn't seem like adding it is worth the effort especially if it means pushing a breaking change to all users. And we have broken tests and a merge conflict on this PR as well.

I'd be happy to discuss further if you do think the impact of this change is a significant enough benefit to outweigh all these drawbacks though!

Thomas-git commented 5 years ago

As I said in my previous reply, I'm OK with your choice. What it means however is that we must throw an error in this case :

li foo
  span bar

Because actually, the parse do not honor the indent : span is not child of li. If you agree with that, I will revert behavior to throwing an error. Once we agree on something, I've also got a fix for #42 #32 #20. And another nasty one I stumble upon.

jescalan commented 5 years ago

Ah ok, I see what you're saying. I think that's ok to throw an error in that case. We can make it something clear like:

Error: Inline text and nested tags cannot both be used at the same time. To resolve this error, nest all content and use a "|" to denote a text node.

If we really wanted to be thorough, we could have the error print out the fix, but it doesn't feel necessary. Does that sound ok?

Thomas-git commented 5 years ago

Yes, that's fine for me. I close this pull request, and I'll come back with another one to fix everything. One commit per fix.