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

Unbuffered comments are treated as block... #1954

Closed
flynx opened this issue May 16, 2015 · 13 comments
Closed

Unbuffered comments are treated as block... #1954

flynx opened this issue May 16, 2015 · 13 comments

Comments

@flynx
Copy link

flynx commented May 16, 2015

Here is a trivial set of code...

mixin foo
  if block
    span there is a block block
    block
  else
    span no block present.

+foo
  //- unbuffered comment...
  //- some code we are not using at the moment...

Output:

<span>there is a block block</span>

Expected:

<span>no block present</span>

This is likely related to #1944

As before, unbuffered comments are treated as blocks while they should be dropped.

P.S. Am I the only one writing documentation for Jade templates? ;)

@ForbesLindesay
Copy link
Member

I'm not sure that I agree in this case. I would argue that the only reason to put an unbuffered comment there is to explicitly crate an empty block.

I don't mind too much though and I'd love to hear what other people think?

@flynx
Copy link
Author

flynx commented May 27, 2015

@ForbesLindesay in most languages there is a no-op for this, like python's pass, comments are generally used for commenting/documenting or temporarily "commenting out" code, in either case they should not affect the code semantics and should be safe to remove without any effect to the code...

IMO Jade's buffered comments are an exception to the above as they are not really Jade comments but rather HTML comments encoded to Jade and thus significant. unbuffered comments on the other hand, as comments, should be safe to remove without changing the code semantics... and that last bit is currently not true, hence issue ;)

@jeromew
Copy link
Contributor

jeromew commented May 27, 2015

I probably would not have expected the unbuffered comment to create a block but I have never used such orphan comments.

If I wanted to created an empty block, I maybe would have written = "" . There are maybe other options to create an empty block.

no sure though if it is worth modifying + it would be a breaking change.

@alubbe
Copy link
Member

alubbe commented May 27, 2015

I agree that this is an unexpected behaviour but seems to be a rather rare edge case. Maybe something to look out for in the v2?

@flynx
Copy link
Author

flynx commented May 27, 2015

@alubbe writing documentation for/in templates is a rare edge case? ....this could explain things))))

@jeromew IMHO when comment placement can break code or change it's semantics, it is a problem (see my other issue #1944).

@alubbe
Copy link
Member

alubbe commented May 28, 2015

It's not an edge case to write documentation inside a template, but the combination of writing documentation (that does not get rendered into HTML) and checking for content existence via the block attribute seems like an edge case to me. But I may be off.

Anyway, I think that if no actual HTML is rendered, the "block" should be falsy.

@flynx
Copy link
Author

flynx commented May 28, 2015

@alubbe this particular example/issue was a gotcha I got commenting out code while testing, i.e. comments affecting semantics, the more specific documentation case quite common in how I write things is in the other issue (#1944)...

here's a snippit:

//- we test for ... here.
if something
  div
//- we do ...
else
  span

....the above code throws a syntax error ;)

@ForbesLindesay
Copy link
Member

@flynx I agree, that shouldn't throw a syntax error. I would accept a pull request to fix that and add a test case.

I have changed my mind and now agree that:

mixin foo
  if block
    span there is a block block
    block
  else
    span no block present.

+foo
  //- unbuffered comment...
  //- some code we are not using at the moment...

should render as:

<span>no block present</span>

This is, however, a breaking change. I agree with @alubbe that this particular example is probably a rare corner case, so the best bet is probably to put this on the 2.0.0 road map.

@ForbesLindesay ForbesLindesay added this to the 2.0.0 milestone May 31, 2015
@flynx
Copy link
Author

flynx commented Jun 5, 2015

@ForbesLindesay OK, I'll dig into the code, but I can't make any promises on the schedule, have to get to my current deadline first ;)

@flynx
Copy link
Author

flynx commented Jun 23, 2015

@ForbesLindesay OK, dove into the code, now cleaning up the tests, should be up within the next several days...

@ForbesLindesay
Copy link
Member

Cool. Looking forward to seeing how you approached this.

@flynx
Copy link
Author

flynx commented Jun 23, 2015

@ForbesLindesay done, ready for review ;)

UPD: just remembered, since Jade is now a bucket of single file modules, the actual pull request is to one of those modules rather than to the main repo, so her's a link for direct reference:
pugjs/pug-parser#8

@flynx
Copy link
Author

flynx commented Jul 17, 2015

@ForbesLindesay any comments? ....couldn't have been this shocking, or could it?! ;)

TimothyGu added a commit that referenced this issue Aug 12, 2015
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging a pull request may close this issue.

5 participants