Inner whitespace nuke is inconsistent with loops #465

Open
nex3 opened this Issue Nov 29, 2011 · 20 comments

Comments

Projects
None yet
10 participants
@nex3
Collaborator

nex3 commented Nov 29, 2011

%ul<
  = "foo"
  = "bar"
  = "baz"

produces

<ul>foobarbaz</ul>

but

%ul<
  - for str in %w[foo bar baz]
    = str

produces

<ul>foo
bar
baz</ul>

richardkmichael added a commit to richardkmichael/haml that referenced this issue Feb 7, 2012

[Haml] Make inner whitespace nuke consistent with loops. #465
%ul<
  - for str in %w[foo bar baz]
    = str

<ul>foobarbaz</ul>
@dennybritz

This comment has been minimized.

Show comment Hide comment
@dennybritz

dennybritz Feb 13, 2012

Good job. It would be great if we could include this ASAP since it keeps bothering me over and over again...

Good job. It would be great if we could include this ASAP since it keeps bothering me over and over again...

@jasonkuhrt

This comment has been minimized.

Show comment Hide comment
@jasonkuhrt

jasonkuhrt Mar 9, 2012

@dennybritz +1

I first reported this bug in #463 and we're still suffering from it to this day.

@dennybritz +1

I first reported this bug in #463 and we're still suffering from it to this day.

@jasonkuhrt

This comment has been minimized.

Show comment Hide comment
@jasonkuhrt

jasonkuhrt Mar 23, 2012

@richardkmichael I noticed something that still does not work:

.fluid-queue<
  = render :partial => "searches/result", :collection => @results

In order to remove whitespace from the above we have to do this:

.fluid-queue<
  - @results.each do |job|
    = render :partial => "searches/api/job_result", :object => job

@richardkmichael Can you confirm the same thing?

@richardkmichael I noticed something that still does not work:

.fluid-queue<
  = render :partial => "searches/result", :collection => @results

In order to remove whitespace from the above we have to do this:

.fluid-queue<
  - @results.each do |job|
    = render :partial => "searches/api/job_result", :object => job

@richardkmichael Can you confirm the same thing?

@richardkmichael

This comment has been minimized.

Show comment Hide comment
@richardkmichael

richardkmichael Mar 24, 2012

Contributor

@jasonkuhrt It's that exact case I was originally looking at with @chooh! Yes, it still doesn't work, but it's on my short list to review.

In fact, I suspect what I've done is rather hackish. In particular, it's worrying I didn't so anything symmetric with "outer whitespace". In this PR, all I was looking for was some comment about the general approach.

Contributor

richardkmichael commented Mar 24, 2012

@jasonkuhrt It's that exact case I was originally looking at with @chooh! Yes, it still doesn't work, but it's on my short list to review.

In fact, I suspect what I've done is rather hackish. In particular, it's worrying I didn't so anything symmetric with "outer whitespace". In this PR, all I was looking for was some comment about the general approach.

@jasonkuhrt

This comment has been minimized.

Show comment Hide comment
@jasonkuhrt

jasonkuhrt Mar 26, 2012

@richardkmichael Oh and in all cases I noticed that I need to set :ugly to true. Not a big deal but again not sure if this is expected or not. Thanks for your contributions to this issue!

@richardkmichael Oh and in all cases I noticed that I need to set :ugly to true. Not a big deal but again not sure if this is expected or not. Thanks for your contributions to this issue!

@jasonkuhrt

This comment has been minimized.

Show comment Hide comment
@jasonkuhrt

jasonkuhrt Apr 19, 2012

@richardkmichael Any luck with checking out the case with :collections ?

@richardkmichael Any luck with checking out the case with :collections ?

@richardkmichael

This comment has been minimized.

Show comment Hide comment
@richardkmichael

richardkmichael Apr 19, 2012

Contributor

It's on my list for next week actually. Sorry this has been languishing.
On Apr 19, 2012 3:18 PM, "Jason Kuhrt" <
reply@reply.github.com>
wrote:

@richardkmichael Any luck with checking out the case with :collections ?


Reply to this email directly or view it on GitHub:
https://github.com/nex3/haml/issues/465#issuecomment-5221271

Contributor

richardkmichael commented Apr 19, 2012

It's on my list for next week actually. Sorry this has been languishing.
On Apr 19, 2012 3:18 PM, "Jason Kuhrt" <
reply@reply.github.com>
wrote:

@richardkmichael Any luck with checking out the case with :collections ?


Reply to this email directly or view it on GitHub:
https://github.com/nex3/haml/issues/465#issuecomment-5221271

@jasonkuhrt

This comment has been minimized.

Show comment Hide comment
@jasonkuhrt

jasonkuhrt Apr 21, 2012

@richardkmichael No worries, was just curious. Great to hear it's still on your radar.

@richardkmichael No worries, was just curious. Great to hear it's still on your radar.

norman added a commit that referenced this issue Apr 30, 2012

Make inner whitespace nuke consistent with loops.
%ul<
  - for str in %w[foo bar baz]
    = str

<ul>foobarbaz</ul>

See #465 for discussion.
Closes #489.

Signed-off-by: Norman Clarke <norman@njclarke.com>

norman added a commit that referenced this issue Apr 30, 2012

@norman

This comment has been minimized.

Show comment Hide comment
@norman

norman Apr 30, 2012

Member

@richardkmichael thanks a lot for working on this. If you can look into the issue with collections that would be great. I already pulled #489 since even without that it's already a nice step forward.

Member

norman commented Apr 30, 2012

@richardkmichael thanks a lot for working on this. If you can look into the issue with collections that would be great. I already pulled #489 since even without that it's already a nice step forward.

@richardkmichael

This comment has been minimized.

Show comment Hide comment
@richardkmichael

richardkmichael Apr 30, 2012

Contributor

Definitely will do; just need to block some time for it.

Contributor

richardkmichael commented Apr 30, 2012

Definitely will do; just need to block some time for it.

@jasonkuhrt

This comment has been minimized.

Show comment Hide comment
@jasonkuhrt

jasonkuhrt May 17, 2012

Hey @richardkmichael, have you had a chance to look yet? I'm just curious if it's a matter of time or the problem is really thorny? Cheers

Hey @richardkmichael, have you had a chance to look yet? I'm just curious if it's a matter of time or the problem is really thorny? Cheers

@richardkmichael

This comment has been minimized.

Show comment Hide comment
@richardkmichael

richardkmichael May 17, 2012

Contributor

I've played around a bit, I don't think it's particularly thorny. Time is a harder problem. ;-)

Contributor

richardkmichael commented May 17, 2012

I've played around a bit, I don't think it's particularly thorny. Time is a harder problem. ;-)

@richardkmichael

This comment has been minimized.

Show comment Hide comment
@richardkmichael

richardkmichael May 22, 2012

Contributor

@jasonkuhrt TL;DR -- I looked at it this afternoon and it's messier than I thought. I won't get to it any time soon.

AFAICT, it's not as simple as adding a nuke_inner_whitespace or nuke_outer_whitespace call.

If you want

%div
  - @objects.each do |object|
    = render :partial => 'object', :object => object

to behave like

%div
  = render :partial => 'object', :collection => @objects

Where the partial contains:

%div>
  = object

Then it means outputting:

    <div>
      <div>
        Block 1
      </div><div>
        Block 2
      </div><div>
        Block 3
      </div><div>
        Block 4
      </div><div>
        Block 5
      </div>
    </div>

Notice, it's not unequivocally nuking whitespace outside the inner div elements. Specifically, the first div appears on a newline, and the last div preserves its trailing newline.

This work is done inside the compiler, (push_script in this case) using the various *_merged_text methods.

https://github.com/haml/haml/blob/master/lib/haml/compiler.rb#L341

I was hoping rstrip_buffer! and a test for nuke_outer_whitespace could be used to eat the offending newline (before appending the next one), but it's not obvious (to me at the moment).

Also, modification might be required to more than the push_script method, because of the %tag= handling.. I think. And testing will be quite important, as changing anything this far inside the compiler might break things. (There are many calls to the *_merged_text methods ; the push_tag method is particularly lengthy.)

Happy to discuss with @norman, if there are any thoughts about it. It would be nice to fix it. OTOH, I'm not using HAML in any project at the moment, and I've never had this use-case. So I'm afraid I'm setting aside for now.

Contributor

richardkmichael commented May 22, 2012

@jasonkuhrt TL;DR -- I looked at it this afternoon and it's messier than I thought. I won't get to it any time soon.

AFAICT, it's not as simple as adding a nuke_inner_whitespace or nuke_outer_whitespace call.

If you want

%div
  - @objects.each do |object|
    = render :partial => 'object', :object => object

to behave like

%div
  = render :partial => 'object', :collection => @objects

Where the partial contains:

%div>
  = object

Then it means outputting:

    <div>
      <div>
        Block 1
      </div><div>
        Block 2
      </div><div>
        Block 3
      </div><div>
        Block 4
      </div><div>
        Block 5
      </div>
    </div>

Notice, it's not unequivocally nuking whitespace outside the inner div elements. Specifically, the first div appears on a newline, and the last div preserves its trailing newline.

This work is done inside the compiler, (push_script in this case) using the various *_merged_text methods.

https://github.com/haml/haml/blob/master/lib/haml/compiler.rb#L341

I was hoping rstrip_buffer! and a test for nuke_outer_whitespace could be used to eat the offending newline (before appending the next one), but it's not obvious (to me at the moment).

Also, modification might be required to more than the push_script method, because of the %tag= handling.. I think. And testing will be quite important, as changing anything this far inside the compiler might break things. (There are many calls to the *_merged_text methods ; the push_tag method is particularly lengthy.)

Happy to discuss with @norman, if there are any thoughts about it. It would be nice to fix it. OTOH, I'm not using HAML in any project at the moment, and I've never had this use-case. So I'm afraid I'm setting aside for now.

@jasonkuhrt

This comment has been minimized.

Show comment Hide comment
@jasonkuhrt

jasonkuhrt May 24, 2012

@richardkmichael Thanks for the update! Cheers to trying =)

@richardkmichael Thanks for the update! Cheers to trying =)

@tonybruess

This comment has been minimized.

Show comment Hide comment
@tonybruess

tonybruess Jan 16, 2014

Any furthered progress on this?

Any furthered progress on this?

@ain

This comment has been minimized.

Show comment Hide comment
@ain

ain Aug 20, 2015

👍 for a fix.

ain commented Aug 20, 2015

👍 for a fix.

@graygilmore

This comment has been minimized.

Show comment Hide comment
@graygilmore

graygilmore Oct 28, 2015

👍

👍

@infinityrobot

This comment has been minimized.

Show comment Hide comment
@infinityrobot

infinityrobot Mar 2, 2016

👍 An update on this would be awesome – unfortunately struggling with this behaviour in a project at the moment.

👍 An update on this would be awesome – unfortunately struggling with this behaviour in a project at the moment.

@k0kubun

This comment has been minimized.

Show comment Hide comment
@k0kubun

k0kubun Feb 28, 2017

Member

While I'm not sure what change fixed this, this bug looks already resolved.

$ cat input.haml
%ul<
  = 'foo'
  = 'bar'
  = 'baz'

%ul<
  - for str in %w[foo bar baz]
    = str
$ haml -v
Haml 5.0.0.beta.2
$ haml input.haml
<ul>foobarbaz</ul>
<ul>foobarbaz</ul>

Please try latest Haml, 5.0.0.beta.2.

Member

k0kubun commented Feb 28, 2017

While I'm not sure what change fixed this, this bug looks already resolved.

$ cat input.haml
%ul<
  = 'foo'
  = 'bar'
  = 'baz'

%ul<
  - for str in %w[foo bar baz]
    = str
$ haml -v
Haml 5.0.0.beta.2
$ haml input.haml
<ul>foobarbaz</ul>
<ul>foobarbaz</ul>

Please try latest Haml, 5.0.0.beta.2.

@k0kubun k0kubun closed this Feb 28, 2017

@k0kubun

This comment has been minimized.

Show comment Hide comment
@k0kubun

k0kubun Feb 28, 2017

Member

Oh, sorry, the current concern of this issue is not description's problem but #465 (comment) or #465 (comment), right? Since I haven't researched this yet, reopening.

Member

k0kubun commented Feb 28, 2017

Oh, sorry, the current concern of this issue is not description's problem but #465 (comment) or #465 (comment), right? Since I haven't researched this yet, reopening.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment