Do we actually need this to be an explicit token? If so I think I would prefer it at the opening tag, not the closing one.

But is there any sane reason to include the newlines of lines containing just whitespace and a non-printing tag?

On 12 Jul 2016, at 00:37, Jonne Haß notifications@github.com wrote:

Do we actually need this to be an explicit token? If so I think I would prefer it at the opening tag, not the closing one.

I agree about the preferred syntax, I just followed the way eRB does it. Not sure what you mean about the explicit token?

But is there any sane reason to include the newlines of lines containing just whitespace and a non-printing tag?

No there isn’t. Currently it preserves the whitespace, but not the newline. I guess it probably should nuke that too.

—
You are receiving this because you authored the thread.
Reply to this email directly, view it on GitHub #2985 (comment), or mute the thread https://github.com/notifications/unsubscribe/AAC0o6t4H3L6E4yhX7lxNry3cyS6jLJMks5qUsWSgaJpZM4JJ3hq.

Thinking about it some more, retroactively deleting whitespace seems like a chore. Maybe it’s better to forego the whole thing a simply do a final sweep deleting all lines containing zero or more whitespace chars?

Michel

On 12 Jul 2016, at 00:44, Michel Benevento michelbenevento@yahoo.com wrote:

On 12 Jul 2016, at 00:37, Jonne Haß <notifications@github.com mailto:notifications@github.com> wrote:

Do we actually need this to be an explicit token? If so I think I would prefer it at the opening tag, not the closing one.

I agree about the preferred syntax, I just followed the way eRB does it. Not sure what you mean about the explicit token?

But is there any sane reason to include the newlines of lines containing just whitespace and a non-printing tag?

No there isn’t. Currently it preserves the whitespace, but not the newline. I guess it probably should nuke that too.

—
You are receiving this because you authored the thread.
Reply to this email directly, view it on GitHub #2985 (comment), or mute the thread https://github.com/notifications/unsubscribe/AAC0o6t4H3L6E4yhX7lxNry3cyS6jLJMks5qUsWSgaJpZM4JJ3hq.

With explicit token I mean -%> vs %> (or <%- vs <%). I think the ideal and intuitive behavior could be:

• foo\n results in foo\n
• foo\n<%= "bar" %>\n results in foo\nbar\n
• foo\n <%= "bar" %> \n results in foo\n bar \n
• foo\n<% "bar" %>\n results in foo\n
• foo\n <% "bar" %> \n results in foo\n
• foo\n<% "bar" %><% "baz" %>\n results in foo\n
• foo\nhi <% "bar" %>\n results in foo\nhi \n
• foo\n<% "bar".each_char do |c| %>\n <%= c %>\n<% end %>\n results in foo\n b\n a\n r\n

and so on, no explicit tag to skip it. I just can't come up with a usecase for when you would want lines with just <% tags to appear in the output. But I wouldn't be opposed adding a new tag to keep them. I just actually think the default behavior should be to not to.

On 12 Jul 2016, at 00:44, Michel Benevento michelbenevento@yahoo.com wrote:

On 12 Jul 2016, at 00:37, Jonne Haß <notifications@github.com mailto:notifications@github.com> wrote:

Do we actually need this to be an explicit token?

I guess you mean why intercept the ‘-‘ char? If so, since Char::Reader doesn’t have a “look_back” method, you’d have to keep track of a previous_char and intercept that inside the ‘%>’ block. I don’t think that is a good idea.

Also, the reason for the trailing -%> syntax in eRB is that it can also be used on output nodes to explicitly remove leading whitespace. Which brings back the discussion on how to go about that.

Michel

I'm not talking about your implementation, I'm trying to discuss the ideal behavior of ECR.

Sorry, I somehow missed your initial reply about the explicit nodes.

If we do it like you suggest, we lose some of the fine grained control an explicit character allows. Also, I'm not sure how you would deal with leading whitespace? I like to think eRB has thought this through well enough.

I agree with @jhass here. Basically, lines that only have a "control" expression should not appear in the output. This is a bit tricky to implement, because you need to consider a few ECR tokens at the same time. I'll try to implement this and send a PR.

I also don't see a compelling reason to keep the "old" behaviour of keeping these newlines, but we could add a way to have them back with a different syntax.

The compelling reason is fine grained control over the output, as well as compatibility with eRB. You are proposing a brute force method that ignores all whitespace significance. Anyway, I am sorry my PR gets treated this way, but best of luck to you guys.

@beno Relax, we are just discussing things, nothing is set in stone yet :-)

Can you provide an example where you'd want to keep newlines of control-only lines? For example:

Items:
<% items.each do |item| %>
  - <%= item %>
<% end %>

Output when items = [1, 2, 3]:

Items: 

  - 1

  - 2

  - 3

When do you want this behaviour?

It's when you suppress control-only lines that you get fine-grained behaviour, not the other way around.

I also don't see a real point to preserve compatibility with eRB, because Crystal is not a Ruby implementation. There's also erubies that behaves differently, which ones should we implement?

                                                             ECR
<%= EXPRESSION %> — Inserts the value of an expression.   supported
  With -%> — Trims the following line break.              not supported
<% CODE %> — Executes code, but does not insert a value.  supported
  With <%- — Trims the preceding indentation.             not supported
  With -%> — Trims the following line break.              supported  in this PR
<%# COMMENT %> — Removed from the final output.           supported
  With -%> — Trims the following line break.              not supported
<%% or %%> — A literal <% or %>, respectively.            ~supported

This is how eRB provides fine grained control. It is not only about newlines, it is also indentation and modifying output behaviour. What if I want

<%= somelongexpressionresultinginfoo -%>
<%= somelongexpressionresultinginbar -%>
<%= somelongexpressionresultinginbaz %>

-> foobarbaz

I'd be forced to jam it all in one line without an explicit token.

@beno Those are very good reasons.

If we implement this, I think we should support trimming both preceding and following indentation in more tokens. The easy way to do it is to store these flags in the tokens themselves, and then when generating the output you'd check these flags and remove the spaces and newlines. I think it's much easier to do it this way then at the lexer level.

What do you think?

I'd also try to avoid invoking string_range to check if -%> is met, you can check it by peeking the next token after - and see if it's %, and then error if a > doesn't come later.

But don't you have to rethink the way ECR currently works then? How can you reason about the end tag when all you do is process characters one by one? You’d have to be looking at a more object oriented approach when you evaluate the tokens in pairs I feel. If so, I don’t think that is easier. But maybe I am missing something.

Michel

On 12 Jul 2016, at 16:48, Ary Borenszweig notifications@github.com wrote:

@beno https://github.com/beno Those are very good reasons.

If we implement this, I think we should support trimming both preceding and following indentation in more tokens. The easy way to do it is to store these flags in the tokens themselves, and then when generating the output you'd check these flags and removing the spaces and newlines. I think it's much easier to do it this way then at the lexer level.

What do you think?

—
You are receiving this because you were mentioned.
Reply to this email directly, view it on GitHub #2985 (comment), or mute the thread https://github.com/notifications/unsubscribe/AAC0o0RMNxh3F6hwY0OCW6IfnWzfx0Exks5qU6lWgaJpZM4JJ3hq.

But you can’t peek twice in a row can you? How does that work?

Michel

On 12 Jul 2016, at 16:53, Ary Borenszweig notifications@github.com wrote:

I'd also try to avoid invoking string_range to check if -%> is met, you can check it by peeking the next token after - and see if it's %, and then error if a > doesn't come later.

—
You are receiving this because you were mentioned.
Reply to this email directly, view it on GitHub #2985 (comment), or mute the thread https://github.com/notifications/unsubscribe/AAC0o_YKkUPdxARnpfr5b1yNMycaWs1zks5qU6pngaJpZM4JJ3hq.

Yes, you need to look at a few tokens at once. But I don't think it's possible to do it just in the lexer, for example if you want to remove preceding space.

You just need to peek once and check for % coming after -. If it's % then > must follow, otherwise it's an error because -% isn't valid in crystal's grammar. The only problem would be if that's inside a string, though I just tried it in ERB and it doesn't work either, so I wouldn't worry about that.

Fixed in #2991