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

Fix regression of .Truncated evaluation in manual summaries #2989

Merged
merged 1 commit into from Feb 19, 2017

Conversation

Projects
None yet
3 participants
@socialhacker
Contributor

socialhacker commented Jan 26, 2017

This fixes the behavior of .Truncated that was introduced with commit
bef496b which was later broken. The
desired behavior is that .Truncated would evaluate to false when there
was nothing after the user defined summary marker.

This also adds a simple unit test to ensure that this feature isn't
broken again. The check for content after the user defined summary
marker is done on the raw content instead of the working copy because
some of the markup renderers add elements after the marker, making it
difficult to determine if there is actually any content.

The behavior (evaluating to false when there is no content, just
summary) is also now documented.

@CLAassistant

This comment has been minimized.

Show comment
Hide comment
@CLAassistant

CLAassistant Jan 26, 2017

CLA assistant check
All committers have signed the CLA.

CLAassistant commented Jan 26, 2017

CLA assistant check
All committers have signed the CLA.

Show outdated Hide outdated hugolib/page.go
// the raw content because different markup engines (rst and asciidoc in
// particular) add div and p elements after the summary marker.
func (p *Page) anyContentAfterUserDefinedSummary() bool {
sections := bytes.Split(p.rawContent, helpers.SummaryDivider)

This comment has been minimized.

@bep

bep Feb 3, 2017

Member

I care about performance, and I'm pretty sure we have done the above split before. So no need to do it again.

@bep

bep Feb 3, 2017

Member

I care about performance, and I'm pretty sure we have done the above split before. So no need to do it again.

This comment has been minimized.

@socialhacker

socialhacker Feb 3, 2017

Contributor

Performance is a good goal, and a nice feature of Hugo, and I'm happy to improve this. This split is not actually done anywhere on the raw content, it is done on the work content once it has been modified by renderContent, but that is too late. The same approximate amount of work is also done on the work content right before renderContent in commonConvert (after shortcode and emoji processing). That is where the is replaced by the internal marker. It should be fine to do this check there instead of on the true raw content, so I'll look into changing how that replacement is done so that the truncate flag information can be computed then.

@socialhacker

socialhacker Feb 3, 2017

Contributor

Performance is a good goal, and a nice feature of Hugo, and I'm happy to improve this. This split is not actually done anywhere on the raw content, it is done on the work content once it has been modified by renderContent, but that is too late. The same approximate amount of work is also done on the work content right before renderContent in commonConvert (after shortcode and emoji processing). That is where the is replaced by the internal marker. It should be fine to do this check there instead of on the true raw content, so I'll look into changing how that replacement is done so that the truncate flag information can be computed then.

hugolib: Fix regression of .Truncated evaluation in manual summaries
This fixes the behavior of .Truncated that was introduced with commit
bef496b which was later broken.  The
desired behavior is that .Truncated would evaluate to false when there
was nothing after the user defined summary marker.

This also adds a simple unit test to ensure that this feature isn't
broken again.  The check for content after the user defined summary
marker is done on the raw content instead of the working copy because
some of the markup renderers add elements after the marker, making it
difficult to determine if there is actually any content.

The behavior (evaluating to false when there is no content, just
summary) is also now documented.
@socialhacker

This comment has been minimized.

Show comment
Hide comment
@socialhacker

socialhacker Feb 7, 2017

Contributor

Hi Bjørn, I've updated to only do the raw data split once, I've also included in this version the removal of some dead code, that might have been a mistake though. Let me know if you would like me to make a separate pull request for the removal.

Contributor

socialhacker commented Feb 7, 2017

Hi Bjørn, I've updated to only do the raw data split once, I've also included in this version the removal of some dead code, that might have been a mistake though. Let me know if you would like me to make a separate pull request for the removal.

@bep bep merged commit 99fbc75 into gohugoio:master Feb 19, 2017

4 checks passed

continuous-integration/appveyor/pr AppVeyor build succeeded
Details
continuous-integration/travis-ci/pr The Travis CI build passed
Details
continuous-integration/wercker Wercker build passed
Details
licence/cla Contributor License Agreement is signed.
Details

@socialhacker socialhacker deleted the socialhacker:fix_truncated_regression branch Feb 19, 2017

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