Skip to content
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

Fix parser error #59

Closed
wants to merge 6 commits into from
Closed

Fix parser error #59

wants to merge 6 commits into from

Conversation

Thomas-git
Copy link
Contributor

Hi,

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

Best regards,
Thomas

without this fix you may encounter unwanted divs :

head:title foo
body.bar

will output
<head><title>foo</title></head>
<body>
<div class="bar">
New test ensuring inline tag context is properly reset when
a new tag is encountered
Witout this fix

li foo
  span bar

result in :

<li>foo</li>
<span>bar</span>

expected :
<li>foo<span>bar</span></li>
@jescalan
Copy link
Member

jescalan commented Jun 4, 2019

Is there any issue with a fix like this?

div
  | foo
  span bar

Previous fix introduce ambiguity :

li:a
  span

is span child of a or li ?

as doc says inline tag is for simple case, this complex
use is forbidden with a SugarmlError
@Thomas-git
Copy link
Contributor Author

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
Copy link
Contributor Author

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
Copy link
Member

jescalan commented Jun 4, 2019

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! #32

@Thomas-git
Copy link
Contributor Author

Thomas-git commented Jun 5, 2019

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
Copy link
Member

jescalan commented Jun 5, 2019

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
Copy link
Contributor Author

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
Copy link
Member

jescalan commented Jun 5, 2019

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
Copy link
Contributor Author

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.

@Thomas-git Thomas-git closed this Jun 7, 2019
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants