gohugoio / hugo

The world’s fastest framework for building websites.
https://gohugo.io
Apache License 2.0
74.72k stars 7.45k forks source link

Add .Page.Contents #8680

Closed regisphilibert closed 2 weeks ago

regisphilibert commented 3 years ago

https://github.com/gohugoio/hugo/issues/8670 might be a good opportunity to regroup content related methods into one object? Currently we have:

Ideally while still supporting the above, we could have:

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

bep commented 3 years ago

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

regisphilibert commented 3 years ago

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 commented 3 years ago

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 commented 3 years ago

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 commented 3 years ago

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 commented 3 years ago

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 commented 3 years ago

What is the motivation behind making .Content a string?

bep commented 3 years ago

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 commented 3 years ago

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 commented 3 years ago

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 commented 3 years ago

You just want plain coffee, say .Coffee!

Should .Content return .Plain?

regisphilibert commented 3 years ago

Should .Content return .Plain?

:D Nope.

bep commented 3 years ago

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 commented 3 years ago

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

bep commented 3 years ago

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 commented 3 years ago

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 commented 3 years ago

"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 commented 3 years ago

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 commented 3 years ago

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 commented 3 years ago

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 commented 3 years ago

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

regisphilibert commented 3 years ago

Is it convenient to drop warnings in the mean time?

bep commented 3 years ago

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 commented 3 years ago

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:

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 commented 3 years ago

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

bep commented 3 years ago

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

regisphilibert commented 3 years ago

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

bep commented 3 years ago

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

Yes, but I like to look at it this way:

{{ with .Contents }}
  {{ with .Render }}
     <p>{{ .Summary }}<p>
     <p>{{ .Content }}<p>
  {{ end }}
{{ end }}

Note that I'm not totally married to the above setup, and I'm particulary not too happy with the Map ... thing. I find that grouping closely things together makes for a cleaner API; it also often helps with the implementation, as related things often depends on each others. I just now noticed that all the Count methods currently actually depends on a rendered output, meaning that the word count in HTML may be less than the word count in the JSON output ...

regisphilibert commented 3 years ago

Ok here are a few suggestions. (Applogy if I read your first intention wrong)

.Render

I'm a big fan of render but I did not realize this would include several nested methods... So I'm ready to give up on logic and favour clarity/continuity

.Contents.Render.Content

Why not just .Contents.Content?

.Contents.Render.Summary.Summary

Why not just .Contents.Summary. I wouldn't mind it being at the root of .Contents if it can simplify this.

.Contents.Render.Summary.Summary.Trunctated

Why not .Contents.SummaryTruncated? This is useful but not so commonly used that we need this perfect.

.Contents.Render.TableOfContents

Why not just .Contents.TableOfContents? Again, favouring continuity

.Map

.Contents.Map.Headings

Why not just .Contents.Headings now that .Contents is a map.

.Contents.Map.RawContent.FrontMatter

Why not just .Contents.FrontMatter which would returns a map. Not sure if a lot of users would need this raw...

.Contents.Map.RawContent.Content

Why not just .Contents.Raw.Content

.Contents.Map.RawContent.ContentExcludingFrontMatter

Why not just .Contents.Raw.Body

bep commented 3 years ago

Yea, you are probably right (and I especially liked the last part of your comment) ...

bep commented 3 years ago
.Contents.TableOfContents
.Contents.Headings

To me, though, the above is totally confusing to me without a sepearation. They kind of represent the same, but ...

regisphilibert commented 3 years ago

What if we lose .TableOfContents. At least not include it in the new .Contents

I think what we should do is create a new internal template which leverages .Contents.Headings and direct users to use it instead of .TableOfContents

{{ template "_internal/toc.html" . }}

For anything more fancy, it's up to them to create their own template for with .Contents.Headings.

regisphilibert commented 3 years ago

It is my personal opinion that as soon as you make .Contents.Headings available, no one will want to use .TableOfContents anymore... and will build their own table of contents.

bep commented 2 years ago

OK, getting back to this one. I currently need a way to signal if there is a codeblock with a given code language (for the mermaid diagrams). I cannot (sadly) do it with .Scratch (I thought about adding a .Store variant of that which does not get reset, but that may be a story for another day).

But I thought I would make it a little more robust and "official", so I wanted to a HasCodeblock method.

I suspect our last discussion stranded because we thought a little bit "too big"/deep.

What if we started with this interface (all the existing methods will still work, of course):

type Contents interface {

    // Render renders the content into the current output format.
    Render() (interface{}, 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

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

    // HasShortcode returns whether the page has a shortcode with the given name.
    HasShortcode(name string) bool

    // HasShortcode returns whether the page has a codeblock with the given lang.
    HasCodeblock(lang string) bool
}

/cc @regisphilibert @jmooring

regisphilibert commented 2 years ago

Yes this is exciting!