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

hugolib: Add parameter option for .Summary #5806

Merged
merged 3 commits into from
Apr 5, 2019
Merged

Conversation

mcdee
Copy link
Contributor

@mcdee mcdee commented Apr 1, 2019

Add the ability to have a summary page variable that overrides
the auto-generated summary. Logic for obtaining summary becomes:

  • if summary divider is present in content use the text above it
  • if summary variables is present in page metadata use that
  • auto-generate summary from first x words of the content

Fixes #5800

Tests have been added to ensure all three cases work as expected. Note that in the tests the summary has markdown elements to ensure that processing occurs correctly; as such RST is explicitly excluded from the list of tested engines in the tests added in hugolib/page_test.go.

There is a discrepancy between the summary when auto-generated and when generated using the explicit delimiter; in the former case the output is plain and in the latter case it is wrapped with <p>. I chose to go for the former option when generating from the summary metadata, as in general the summary would be wrapped in <div> or similar anyway, and it's cleaner for RSS output.

Talking of which, I added a custom summary to one of the test pages in hugolib/site_test.go to ensure that the output XML is correct with summary metadata that contains markdown; test is in hugolib/rss_test.go.

I haven't studied the Hugo codebase to any great depth so please do take a good look at the changes and let me know if there is anything that's incorrect here. Thanks.

Add the ability to have a `summary` page variable that overrides
the auto-generated summary.  Logic for obtaining summary becomes:

  * if summary divider is present in content use the text above it
  * if summary variables is present in page metadata use that
  * auto-generate summary from first _x_ words of the content

Fixes gohugoio#5800
@bep
Copy link
Member

bep commented Apr 2, 2019

Thanks. There are some tests that fail.

@mcdee
Copy link
Contributor Author

mcdee commented Apr 4, 2019

Okay, the issue was that the tests were in Markdown source and although I excluded the RST processor from the tests I missed the Asciidoc one due to not having it installed locally. Tests should be passing now.

DocumentID: p.File().UniqueID(), DocumentName: p.File().Path(),
Config: cp.p.getRenderingConfig()})
// Strip enclosing <p>
html = []byte(strings.TrimSpace(string(html)))
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

These multiple []byte conversions look slightly ineffective. I would probably just convert it to a string as a first step (as that is what it's going to end up as) and do the trimming on that ...

That said, we've had some discussions on this before, whether we should trim p tags or not.

I'm not totally sure what we do in the "summary split" case, but the code I read here will not work as a general case, so I think that needs to be adjusted/removed. The main problem is the handling of multiple paragraphs. I suggest that we get it in line with the other Markdown summary option.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Second thought, if you pull the logic in

// Strip if this is a short inline type of text.

Into some common function, and then use that.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Would you prefer the common function to remain in transform.go or move to helpers/?

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Move it to helpers/content.go (it cannot live in transform.go anyway because that will create a cyclic reference)

Copy link
Member

@bep bep Apr 4, 2019

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Add it as a method on the ContentSpec struct to make less global helper funcs. You may have to add some whitespace trimming to that logic.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Done. I was tempted to let the function trim all HTML passed in to it but that would change existing output so I stuck to principle of least surprise and only do so when the input is recognised as a single paragraph.

@kaushalmodi
Copy link
Contributor

@mcdee Please update the documentation too .. grep for Summary in the existing docs and makes changes related to this feature wherever appropriate. It would also be really useful to specify the new summary front-matter variable somewhere.

Thanks.

@mcdee
Copy link
Contributor Author

mcdee commented Apr 5, 2019

@kaushalmodi I was planning to do that after this PR was accepted; didn't want to write docs against a moving target.

@bep bep merged commit 3a62d54 into gohugoio:master Apr 5, 2019
@bep
Copy link
Member

bep commented Apr 5, 2019

@mcdee This looks just right, thanks for this. I merged this quickly to get it into my ongoing manual testing for Hugo 0.55 (a rather massive and cool release). I would really appreciate if you could update the docs (in the /docs folder in this repo) in a separate PR with the summary changes.

@github-actions
Copy link

This pull request has been automatically locked since there has not been any recent activity after it was closed. Please open a new issue for related bugs.

@github-actions github-actions bot locked as resolved and limited conversation to collaborators Jan 30, 2022
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Fall back to summary set in front matter
3 participants