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

Add .Page.Contents #8680

Closed
regisphilibert opened this issue Jun 21, 2021 · 36 comments · Fixed by #12759
Closed

Add .Page.Contents #8680

regisphilibert opened this issue Jun 21, 2021 · 36 comments · Fixed by #12759

Comments

@regisphilibert
Copy link
Member

regisphilibert commented Jun 21, 2021

#8670 might be a good opportunity to regroup content related methods into one object? Currently we have:

  • .FuzzyWordCount
  • .RawContent
  • .Plain
  • .PlainWords
  • . ReadingTime
  • .ToC
  • .WordCount
  • .ContentMap (proposed)

Ideally while still supporting the above, we could have:

  • .Content.Raw
  • .Content.Plain
  • .Content.PlainWords
  • .Content.WordCount
  • .Content.WordCountFuzzy
  • .Content.ReadingTime
  • .Content.Rendered (alias for .Content)
  • .Content.TableOfContent
  • .Content.Map

Note that this could be done in steps when a current method name is changed or parameters/UX is improved.

@bep
Copy link
Member

bep commented Jun 21, 2021

Yea, but it sounds like a lot of work (work being the deprecation dance).

@regisphilibert
Copy link
Member Author

Then again, we don't have to do this all at once. But if any of those can benefit from a renaming or any kind of dangerous change, we could move them into this new map instead of changing the old ones.

@bep
Copy link
Member

bep commented Jun 21, 2021

I would be all over this if you could get me a great new container name that we could use for this, e.g. .Contents...

@regisphilibert
Copy link
Member Author

regisphilibert commented Jun 21, 2021

Does it have to be new though? Could the current .Content co-exist with Content.{anything} like the root context of a taxonomy term returns WeightedPages while its .Page method returns its page?

I think we have this for resources types as well.

@bep
Copy link
Member

bep commented Jun 21, 2021

Nevermind, @regisphilibert -- and thanks for bringing this up.

I just did a test at https://play.golang.org/p/LiN4WdipGsv

As long as .Content prints like before and behaves like a string whenever needed, we can make .Content a struct and attach whatever methods to it.

What do you think @moorereason ?

@bep
Copy link
Member

bep commented Jun 21, 2021

OK, there is one challenge ... Content currently is of type template.HTML -- making it a string leads to all kinds of escaping issues. Hmm...

@regisphilibert
Copy link
Member Author

What is the motivation behind making .Content a string?

@bep
Copy link
Member

bep commented Jun 21, 2021

What is the motivation behind making .Content a string?

No, that isn't something we're going to do ... That was just one of my technical ramblings ... The above would work in a mostly backwards compatible way (if we looked past the HTML vs String thing) if we sad that Content.String() was a thing...

We can still do this, as we have some monkey patches of the Go HTML template package already. Esp. if we say that this is a temporary thing -- that we eventually will transition all examples etc. to get from:

{{ .Content }}

To:

{{ .Content.Rendered }}

I'm not totally fan of Rendered, but ...

@regisphilibert
Copy link
Member Author

regisphilibert commented Jun 22, 2021

I'm not totally fan of Rendered, but ...

But we're already using the term .RenderString for the exact same thing. So for the sake of consistency I'm pushing for .Rendered.

But my preferred choice is to keep using .Content for the most used and most obvious thing .Content should do. I like the UX/DX that if you want the obvious thing, just use .Content but if you need more refined contenty things, you'll find them under .Content.{method}.

It might make people cringe that a keyword can be both a method and "map/object" but I like it a lot.

You just want plain regular coffee, say .Coffee! You want a decaf say .Coffee.Decaf!

@regisphilibert
Copy link
Member Author

One thing of importance though: I think (I know I am) a lot of people are using {{ with .Content }} to check if there's anything in the markdown body. We might have to make sure this also returns false when there's no content to avoid breaking changes.

@jmooring
Copy link
Member

You just want plain coffee, say .Coffee!

Should .Content return .Plain?

@regisphilibert
Copy link
Member Author

Should .Content return .Plain?

:D Nope.

@bep
Copy link
Member

bep commented Jun 22, 2021

One thing of importance though: I think (I know I am) a lot of people are using {{ with .Content }}

That looks like a showstopper. Even with empty content, this should work:

{{ with .Content }}
{{ .WordCount }}
{{ end }}

@regisphilibert
Copy link
Member Author

Yes. Safer to find a new keyword then... Back to "what's better than .Content..."

@bep
Copy link
Member

bep commented Jun 29, 2021

But ... if you look at https://dictionary.cambridge.org/grammar/british-grammar/content-or-contents

So, I'm Norwegian, but to me it looks like we may be saved by a typo here, that the correct term should have been to use the plural .Contents all along?

/cc @jmooring

@regisphilibert
Copy link
Member Author

regisphilibert commented Jun 29, 2021

I'm good with that! Regardless of one's grammar knowledge, plural intuitively let the user know that there is several stuff in there!

@jmooring
Copy link
Member

"Content" is correct, meaning "the ideas that are contained in a piece of writing." We are reaching for alternatives because "Content" is already in use.

config.toml

baseURL = "http://example.org/"
languageCode = "en-us"
title = "My Site"
api = "2.0"  # Introduce breaking changes on an opt-in basis

@regisphilibert
Copy link
Member Author

We are reaching for alternatives because "Content" is already in use.

Correct, but Contents plural still make sense as to convey not only some text but more data surrounding it.

@bep
Copy link
Member

bep commented Jun 29, 2021

api = "2.0" # Introduce breaking changes on an opt-in basis

If we do the above that will be the new default (I have never heard of a software project that stuck with API1 as the default when releasing API2) -- so people would have to opt out of it to get there. If people had used the "max hugo version" in their site config actively we could use that. But the current situation tells me that our best bet if we want to do this is to add a new method (e.g. .Contents)

@regisphilibert
Copy link
Member Author

I agree we should just move on and add this a new a method while keeping the old ones for awhile.
And (unless technically advised against) I don't think we should ever remove .Content. This works as a nice alias for .Contents.Rendered and is used on hundreds of theme already.

@bep
Copy link
Member

bep commented Jun 29, 2021

We can keep .Content, but we need to eventually get rid of .WordCount and similar.

@regisphilibert
Copy link
Member Author

Is it convenient to drop warnings in the mean time?

@bep
Copy link
Member

bep commented Jun 29, 2021

Is it convenient to drop warnings in the mean time?

Yes, I have a system in place for that (warning first for a few versions, then error).

@bep
Copy link
Member

bep commented Jul 1, 2021

I have thinkered a little about this (see below). Don't look too hard on the interface names; but the core interface is Contents which would be return in .Page.Content.

Some of my thoughts behind the below:

  • I use the imperative mood when applicable, e.g. Render and not Rendered. This fits nicely with the lazy approach of Hugo, we render on command. But it also sorts/groups nicely CountWords, Count... when documented.
  • I'm not totally sure about the placement of RawContent, but I think it's smart to make it its own thing.
package page

import (
	"html/template"

	"github.com/gohugoio/hugo/markup/tableofcontents"
)

type Contents interface {

	// Render renders the content into the current output format.
	Render() (ContentsContent, error)

	// Plain returns the content as plain text with no markup.
	Plain() string

	// PlainWords returns the content split into words with no markup.
	PlainWords() []string

	CountWords() int
	CountWordsFuzzy() int
	ReadingTime() int

	// Remove this.
	Len() int

	Map() ContentsMap
}

type ContentsMap interface {
	// Headings returns a recursive list of headings. Use this to render
	// your custom ToC.
	Headings() tableofcontents.Headings

	// RawContent gives access to the raw, unprocessed content.
	RawContent() ContentsRaw
}

type ContentsRaw interface {
	// FrontMatter gives you the front matter, if set. Passing this
	// to transform.Unmarshal will give you a map.
	FrontMatter() []bytes

	// Content returns the raw unprocessed content, including any front matter.
	Content() []bytes

	// ContentExcludingFrontMatter returns the raw unprocessed content stripped
	// of any front matter.
	ContentExcludingFrontMatter() []bytes
}

type ContentsContent interface {
	Content() []byte
	Summary() ContentSummary
	TableOfContents() template.HTML
}

type ContentSummary interface {
	Summary() template.HTML
	Truncated() bool
}

@regisphilibert
Copy link
Member Author

Yeah I realize my own illustration is really not clear either...

@bep
Copy link
Member

bep commented Jul 1, 2021

I'm not too good at reading your file 100% correctly ...

@regisphilibert
Copy link
Member Author

By my approximative interpretation it seems in order to print the current .Content we need to {{ .Contents.Render.Content }} is that correct?

bep added a commit to bep/hugo that referenced this issue Aug 13, 2024
bep added a commit to bep/hugo that referenced this issue Aug 14, 2024
bep added a commit to bep/hugo that referenced this issue Aug 14, 2024
bep added a commit to bep/hugo that referenced this issue Aug 16, 2024
bep added a commit to bep/hugo that referenced this issue Aug 17, 2024
bep added a commit to bep/hugo that referenced this issue Aug 23, 2024
bep added a commit to bep/hugo that referenced this issue Aug 25, 2024
bep added a commit to bep/hugo that referenced this issue Aug 28, 2024
bep added a commit to bep/hugo that referenced this issue Aug 28, 2024
bep added a commit to bep/hugo that referenced this issue Aug 28, 2024
bep added a commit to bep/hugo that referenced this issue Aug 28, 2024
bep added a commit to bep/hugo that referenced this issue Aug 28, 2024
bep added a commit to bep/hugo that referenced this issue Aug 29, 2024
bep added a commit to bep/hugo that referenced this issue Aug 29, 2024
bep added a commit to bep/hugo that referenced this issue Aug 29, 2024
Note that this also adds a new `.ContentWithoutSummary` method, and to do that we had to unify the different summary types:

Both `auto` and `manual` now returns HTML. Before this commit, `auto` would return plain text. This could be considered to be a slightly breaking change, but for the better: Now you can treat the `.Summary` the same without thinking about where it comes from, and if you want plain text, pipe it into `{{ .Summary | plainify }}`.

Fixes gohugoio#8680
Fixes gohugoio#12761
Fixes gohugoio#12778
Fixes gohugoio#716
bep added a commit to bep/hugo that referenced this issue Aug 29, 2024
Note that this also adds a new `.ContentWithoutSummary` method, and to do that we had to unify the different summary types:

Both `auto` and `manual` now returns HTML. Before this commit, `auto` would return plain text. This could be considered to be a slightly breaking change, but for the better: Now you can treat the `.Summary` the same without thinking about where it comes from, and if you want plain text, pipe it into `{{ .Summary | plainify }}`.

Fixes gohugoio#8680
Fixes gohugoio#12761
Fixes gohugoio#12778
Fixes gohugoio#716
bep added a commit to bep/hugo that referenced this issue Aug 29, 2024
Note that this also adds a new `.ContentWithoutSummary` method, and to do that we had to unify the different summary types:

Both `auto` and `manual` now returns HTML. Before this commit, `auto` would return plain text. This could be considered to be a slightly breaking change, but for the better: Now you can treat the `.Summary` the same without thinking about where it comes from, and if you want plain text, pipe it into `{{ .Summary | plainify }}`.

Fixes gohugoio#8680
Fixes gohugoio#12761
Fixes gohugoio#12778
Fixes gohugoio#716
bep added a commit to bep/hugo that referenced this issue Aug 29, 2024
Note that this also adds a new `.ContentWithoutSummary` method, and to do that we had to unify the different summary types:

Both `auto` and `manual` now returns HTML. Before this commit, `auto` would return plain text. This could be considered to be a slightly breaking change, but for the better: Now you can treat the `.Summary` the same without thinking about where it comes from, and if you want plain text, pipe it into `{{ .Summary | plainify }}`.

Fixes gohugoio#8680
Fixes gohugoio#12761
Fixes gohugoio#12778
Fixes gohugoio#716
bep added a commit to bep/hugo that referenced this issue Aug 29, 2024
Note that this also adds a new `.ContentWithoutSummary` method, and to do that we had to unify the different summary types:

Both `auto` and `manual` now returns HTML. Before this commit, `auto` would return plain text. This could be considered to be a slightly breaking change, but for the better: Now you can treat the `.Summary` the same without thinking about where it comes from, and if you want plain text, pipe it into `{{ .Summary | plainify }}`.

Fixes gohugoio#8680
Fixes gohugoio#12761
Fixes gohugoio#12778
Fixes gohugoio#716
bep added a commit to bep/hugo that referenced this issue Aug 29, 2024
Note that this also adds a new `.ContentWithoutSummary` method, and to do that we had to unify the different summary types:

Both `auto` and `manual` now returns HTML. Before this commit, `auto` would return plain text. This could be considered to be a slightly breaking change, but for the better: Now you can treat the `.Summary` the same without thinking about where it comes from, and if you want plain text, pipe it into `{{ .Summary | plainify }}`.

Fixes gohugoio#8680
Fixes gohugoio#12761
Fixes gohugoio#12778
Fixes gohugoio#716
bep added a commit to bep/hugo that referenced this issue Aug 29, 2024
Note that this also adds a new `.ContentWithoutSummary` method, and to do that we had to unify the different summary types:

Both `auto` and `manual` now returns HTML. Before this commit, `auto` would return plain text. This could be considered to be a slightly breaking change, but for the better: Now you can treat the `.Summary` the same without thinking about where it comes from, and if you want plain text, pipe it into `{{ .Summary | plainify }}`.

Fixes gohugoio#8680
Fixes gohugoio#12761
Fixes gohugoio#12778
Fixes gohugoio#716
bep added a commit to bep/hugo that referenced this issue Aug 29, 2024
Note that this also adds a new `.ContentWithoutSummary` method, and to do that we had to unify the different summary types:

Both `auto` and `manual` now returns HTML. Before this commit, `auto` would return plain text. This could be considered to be a slightly breaking change, but for the better: Now you can treat the `.Summary` the same without thinking about where it comes from, and if you want plain text, pipe it into `{{ .Summary | plainify }}`.

Fixes gohugoio#8680
Fixes gohugoio#12761
Fixes gohugoio#12778
Fixes gohugoio#716
@bep bep closed this as completed in 3760926 Aug 29, 2024
@bep bep modified the milestones: v0.133.0, v0.134.0 Aug 29, 2024
Copy link

This issue 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 Sep 20, 2024
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Projects
None yet
Development

Successfully merging a pull request may close this issue.

3 participants