Skip to content
New issue

Have a question about this project? # for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “#”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? # to your account

Human readable format (v2) #64

Merged
merged 2 commits into from
Dec 15, 2015
Merged

Human readable format (v2) #64

merged 2 commits into from
Dec 15, 2015

Conversation

jnvsor
Copy link
Collaborator

@jnvsor jnvsor commented Dec 12, 2015

This is another PR for "Human readable format" now renamed ignore.blanklines

This one is shorter and simpler, and addresses some questions with the original PR.

Why not use ignore.whitespaces?

No combination of ignore.whitespaces actually removes blank lines and only blank lines. While you can disable specific types of whitespace, that's all it does (As far as I can tell, correct if I'm wrong)

Using something like my jekyll menu as an example, the uncompressed version generates 100 blank lines for every line with content. This is the horror of liquid, and one of the reasons JCH is so popular :)

Removing only newlines would lead to several hundred spaces between content, and removing only spaces would still leave 100 blank lines (Though with less spaces I suppose it would have a smaller file size)

I can't think many people want the ability to remove arbitrary whitespace - certainly not as many as want to remove blank lines.

Performance concerns of moving comment parsing above whitespace collapse

While moving comment parsing above whitespace collapse does have a small (Less than 1.5% by my bench) performance penalty, getting rid of ignore.whitespaces offsets that and goes on to give a further 10% performance improvement.

These benches came from Jekyll 3 but Jekyll 2.0 showed similar numbers.

Edit: I just checked 866399a from before either of our whitespace patches and there's still a good 10% performance improvement over that in this patch. Probably due to other changes that occurred in the meanwhile like changing the case to a pair of ifs in a capture.

Subjectiveness of "Human readable"

human readable is more or less a matter of taste. Other humans will raise new issues to match their readability standards. I think on all the endless discussions about indent style.

As the switch in this patch only removes blank lines and leaves other whitespace alone, this will not affect the output enough to raise indent style issues (Indents will be the same as the source file), and it's simple enough not to raise other issues (We're not claiming to parse and rewrite the DOM here!)

@doktorbro
Copy link
Collaborator

What a great analysis. You are right, the new option is more appropriate than the ignore.whitespaces. I like the performance gain too.

What I don’t understand: why did you put the option inside the ignore namespace? The option turns on the collapsing of blank lines only, so I would call it blanklines, without the ignore.

compress_html:
  blanklines: true

Would you mind to remove the ignore namespace?

@jnvsor
Copy link
Collaborator Author

jnvsor commented Dec 15, 2015

Done

doktorbro pushed a commit that referenced this pull request Dec 15, 2015
Human readable format (v2)
@doktorbro doktorbro merged commit 5ee9d69 into penibelst:master Dec 15, 2015
@doktorbro
Copy link
Collaborator

Great.

@jnvsor jnvsor deleted the hr2 branch December 15, 2015 18:26
# for free to join this conversation on GitHub. Already have an account? # to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants