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

Re: Comments / Whitespace - standalone delimited comments left. #91

Open
swizca opened this issue Jan 30, 2017 · 6 comments
Open

Re: Comments / Whitespace - standalone delimited comments left. #91

swizca opened this issue Jan 30, 2017 · 6 comments
Labels

Comments

@swizca
Copy link

swizca commented Jan 30, 2017

I appreciate:

compress_html:
comments: ["<!-- ", " -->"]
Whitespaces around the tags prevent conditional comments from being deleted.

So I have, for example, made some comments like this <!--| comment |-->.

<!-- comment -->'s are indeed deleted, however <!--{eol} comment {bol}-->, where bol/eol = beginning / end of line, are still present.

Shouldn't the whitespaced stripped comment delimiters (also) be used?

[Even if <--{space}{eol} is used, editors frequently strip whitespace from {eol}, defeating this intent.]

@jnvsor
Copy link
Collaborator

jnvsor commented Jan 30, 2017

Good point. IIRC I had a way to fix that once but it tanked performance. For a quick fix you could try something like:

<!-- .
comment
. -->

Or you could change it to <!---

@jnvsor jnvsor added the bug label Jan 30, 2017
@swizca
Copy link
Author

swizca commented Jan 30, 2017

Or you could change it to <!---

Reverses the norm. <--| is trying to except certain comments, not force one to re-jigger all comments except the ones one wants to keep. Let alone imported code ...

Almost feels like a reverse-comments (keep_comments) setting would be useful.

@swizca
Copy link
Author

swizca commented Jan 30, 2017

Actually ... the same occurs with <!--{tab} (and {space}--> has no impact).

So, it would seem that a provided comment: should be preprocessed to munge any whitespace to mean all whitespace.

Or ... the provided comment: to accept / presume RE's. (?)

This would allow <!--[^\|] , in these examples, and <!--[^\[] would accommodate the current documented edge case. (Conditional if's.)

No?

@jnvsor
Copy link
Collaborator

jnvsor commented Jan 31, 2017

That preprocessing is what's going to kill your performance, and liquid doesn't support regex. (If it did this project would be way way easier :P)

A better solution would be to look for <!-- and ensure it's not followed by [if but I dread to think what it'll do to perf

@swizca
Copy link
Author

swizca commented Jan 31, 2017

liquid doesn't support regex.

Would seem to be the operative problem.

Unless one could introduce / intervene an additional interpreter. e.g. grep scripts. Good luck getting that into a github approved plugin. Interesting, here, with html_compress, is the level of flexibility offered - unlike jekyll-tidy (htmlbeautifier / htmlcompressor) - which have few / no options.

A better solution would be to look for <!-- and ensure it's not followed by [if but I dread to think what it'll do to perf

I take your point. It also points out the problem of how to express intent.

e.g. Your example really means "all <!-- unless followed by ['. Which isn't easily expressed in the current comment: ["{boc}" "{eoc}"] syntax.

Either a keep comment delimiter keep_comment_delims: ["[" "]"] or a comment exceptor keep_comments: ["<!--[" "]-->"] seems needed.

And, given that ' -->' or '-->' appear to have no impact, I wonder if specifying the end comment delimiter is actually useful. Always take (or not) from the given comment starter to '-->'? [One may want to consistently use a matching comment ender, but that is a writing convention, not a coding requirement?]

@jnvsor
Copy link
Collaborator

jnvsor commented Jan 31, 2017

Either a keep comment delimiter keep_comment_delims: ["[" "]"] or a comment exceptor keep_comments: [""] seems needed.

Well yes but how would you implement this? This is liquid! It's the most annoying "Language" to do anything complicated in!

Currently comments, pre tags and the like are implemented by string splitting since that's the only performant way to find a string in another one in liquid (The alternative being to loop character by character that more than triples runtime IIRC)

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

No branches or pull requests

2 participants