~adnano/kiln-devel

This thread contains a patchset. You're looking at the original emails, but you may wish to use the patch review UI. Review patch
3 2

[PATCH] tasks: add tasks.feeds support

Details
Message ID
<20210831143726.38689-1-git@olivier.pfad.fr>
DKIM signature
missing
Download raw message
Patch: +78 -4
Every task can now have a [[tasks.feeds]] entry to generate one or more feeds.
It must have the following arguments:

[[tasks.feeds]]
input_dir = ""
title = "Example feed"
template = "atom.xml"
output = "atom.xml"
---
After our discussion on kiln-discuss, here is my attempt to support the 
[[tasks.feeds]] configuration.

A couple of things must be done after (or before) this is merged:
- update the documentation
- deprecate the current [[feeds]] config

Let me know what you think!

 config.toml                 |  9 ++++--
 dir.go                      | 63 +++++++++++++++++++++++++++++++++++++
 site.go                     |  8 +++++
 templates/_default/atom.xml |  2 +-
 4 files changed, 78 insertions(+), 4 deletions(-)

diff --git a/config.toml b/config.toml
index 7fd55af..9aeb85d 100644
--- a/config.toml
+++ b/config.toml
@@ -1,9 +1,6 @@
title = "Example website"
urls = ["gemini://example.com"]

[feeds]
"/" = "Example feed"

[permalinks]
"/" = "/{{ .Date.Format `2006/01/02` }}/{{ path.Base .Permalink }}/"

@@ -13,3 +10,9 @@ output = ".gmi"
template = ".gmi"
static_dir = "static"
output_dir = "public"

[[tasks.feeds]]
input_dir = ""
title = "Example feed"
template = "atom.xml"
output = "atom.xml"
diff --git a/dir.go b/dir.go
index e0a2505..9d34cc7 100644
--- a/dir.go
+++ b/dir.go
@@ -23,6 +23,8 @@ type Dir struct {
	Dirs      []*Dir
	index     *Page  // The index page.
	feed      []byte // Atom feed.

	extraFiles map[string][]byte // extra generated files
}

// Page represents a page.
@@ -151,6 +153,18 @@ func (d *Dir) _read(srcDir, path string, task *Task, cfg *Site) error {

// process processes the directory's contents.
func (d *Dir) process(cfg *Site, task *Task) error {
	// build feeds
	for _, feed := range task.Feeds {
		if strings.Trim(d.Permalink, "/") != strings.Trim(feed.InputDir, "/") {
			continue
		}
		b, err := d.buildFeed(cfg, feed)
		if err != nil {
			return err
		}
		d.addExtraFile(feed.Output, b)
	}

	if task.TemplateExt != "" {
		// Create index
		if d.index != nil {
@@ -214,6 +228,39 @@ func (d *Dir) process(cfg *Site, task *Task) error {
	return nil
}

// buildFeed build the feed of the directory
func (d Dir) buildFeed(cfg *Site, feed Feed) ([]byte, error) {
	tmpl, ok := cfg.templates.FindTemplate(d.Permalink, feed.Template)
	if !ok {
		return nil, fmt.Errorf("missing feed template \"%s\" to generate \"%q\"", feed.Template, feed.Title)
	}

	var b bytes.Buffer
	data := struct {
		Title     string    // Feed title.
		Permalink string    // Feed permalink.
		Updated   time.Time // Last updated time.
		Pages     []*Page   // Feed pages.
	}{
		Title:     feed.Title,
		Permalink: d.Permalink,
		Updated:   time.Now(),
		Pages:     d.Pages,
	}
	if err := tmpl.Execute(&b, data); err != nil {
		return nil, err
	}
	return b.Bytes(), nil
}

func (d *Dir) addExtraFile(name string, content []byte) {
	if d.extraFiles == nil {
		d.extraFiles = map[string][]byte{name: content}
	} else {
		d.extraFiles[name] = content
	}
}

// write writes the directory's contents to the provided destination path.
func (d *Dir) write(dstDir string, task *Task) error {
	dirPath := pathpkg.Join(dstDir, d.Permalink)
@@ -255,6 +302,22 @@ func (d *Dir) write(dstDir string, task *Task) error {
		}
	}

	for name, content := range d.extraFiles {
		if strings.HasPrefix(name, "/") {
			// if name starts with /, put it at the root
			name = strings.TrimPrefix(name, "/")
		} else {
			// otherwise put it under d.Permalink
			name = pathpkg.Join(d.Permalink, name)
		}

		dstPath := pathpkg.Join(dstDir, name)
		os.MkdirAll(dirPath, 0755)
		if err := os.WriteFile(dstPath, content, 0644); err != nil {
			return err
		}
	}

	// Write subdirectories
	for _, dir := range d.Dirs {
		dir.write(dstDir, task)
diff --git a/site.go b/site.go
index 925ece7..31dd3c2 100644
--- a/site.go
+++ b/site.go
@@ -31,6 +31,14 @@ type Task struct {
	StaticDir   string            `toml:"static_dir"`  // static file directory
	OutputDir   string            `toml:"output_dir"`  // output directory
	UglyURLs    bool              `toml:"ugly_urls"`   // whether to use ugly URLs
	Feeds       []Feed            `toml:"feeds"`
}

type Feed struct {
	InputDir string `toml:"input_dir"`
	Title    string `toml:"title"`
	Template string `toml:"template"`
	Output   string `toml:"output"`
}

func (t *Task) Match(ext string) bool {
diff --git a/templates/_default/atom.xml b/templates/_default/atom.xml
index 89cdd2a..553da4b 100644
--- a/templates/_default/atom.xml
+++ b/templates/_default/atom.xml
@@ -4,7 +4,7 @@
<title>{{ .Title }}</title>
<updated>{{ .Updated.Format "2006-01-02T15:04:05Z07:00" }}</updated>
<link href="{{ index site.URLs 0 | safeURL }}{{ .Permalink }}" rel="alternate"/>
{{ range .Entries }}<entry>
{{ range .Pages }}<entry>
	<id>{{ index site.URLs 0 }}{{ .Permalink }}</id>
	<title>{{ .Title }}</title>
	<updated>{{ .Date.Format "2006-01-02T15:04:05Z07:00" }}</updated>
-- 
2.33.0
Details
Message ID
<CDXUV3LR2PB9.22II9NO3F87ZO@nitro>
In-Reply-To
<20210831143726.38689-1-git@olivier.pfad.fr> (view parent)
DKIM signature
pass
Download raw message
On Tue Aug 31, 2021 at 10:37 AM EDT, oliverpool wrote:
> Every task can now have a [[tasks.feeds]] entry to generate one or more feeds.
> It must have the following arguments:
>
> [[tasks.feeds]]
> input_dir = ""
> title = "Example feed"
> template = "atom.xml"
> output = "atom.xml"
> ---
> After our discussion on kiln-discuss, here is my attempt to support the 
> [[tasks.feeds]] configuration.
>
> A couple of things must be done after (or before) this is merged:
> - update the documentation
> - deprecate the current [[feeds]] config
>
> Let me know what you think!

Thanks! This looks like a good start.

>  config.toml                 |  9 ++++--
>  dir.go                      | 63 +++++++++++++++++++++++++++++++++++++
>  site.go                     |  8 +++++
>  templates/_default/atom.xml |  2 +-
>  4 files changed, 78 insertions(+), 4 deletions(-)
>
> diff --git a/config.toml b/config.toml
> index 7fd55af..9aeb85d 100644
> --- a/config.toml
> +++ b/config.toml
> @@ -1,9 +1,6 @@
>  title = "Example website"
>  urls = ["gemini://example.com"]
>  
> -[feeds]
> -"/" = "Example feed"
> -
>  [permalinks]
>  "/" = "/{{ .Date.Format `2006/01/02` }}/{{ path.Base .Permalink }}/"
>  
> @@ -13,3 +10,9 @@ output = ".gmi"
>  template = ".gmi"
>  static_dir = "static"
>  output_dir = "public"
> +
> +[[tasks.feeds]]
> +input_dir = ""
> +title = "Example feed"
> +template = "atom.xml"
> +output = "atom.xml"
> diff --git a/dir.go b/dir.go
> index e0a2505..9d34cc7 100644
> --- a/dir.go
> +++ b/dir.go
> @@ -23,6 +23,8 @@ type Dir struct {
>  	Dirs      []*Dir
>  	index     *Page  // The index page.
>  	feed      []byte // Atom feed.
> +
> +	extraFiles map[string][]byte // extra generated files

Perhaps this field should be named 'feeds' to make it clear what purpose
it serves. Eventually we'll also want to remove the current 'feed'
field. I don't think we'll be needing to store any other extra files any
time soon.

>  }
>  
>  // Page represents a page.
> @@ -151,6 +153,18 @@ func (d *Dir) _read(srcDir, path string, task *Task, cfg *Site) error {
>  
>  // process processes the directory's contents.
>  func (d *Dir) process(cfg *Site, task *Task) error {
> +	// build feeds
> +	for _, feed := range task.Feeds {
> +		if strings.Trim(d.Permalink, "/") != strings.Trim(feed.InputDir, "/") {

I think it might be better to require an exact match with the directory
permalink, as that is how the configuration for [permalinks] and the
(soon to be deprecated) [feeds] works. Alternatively, we could bring
those in line with this behavior.

> +			continue
> +		}
> +		b, err := d.buildFeed(cfg, feed)
> +		if err != nil {
> +			return err
> +		}
> +		d.addExtraFile(feed.Output, b)
> +	}
> +
>  	if task.TemplateExt != "" {
>  		// Create index
>  		if d.index != nil {
> @@ -214,6 +228,39 @@ func (d *Dir) process(cfg *Site, task *Task) error {
>  	return nil
>  }
>  
> +// buildFeed build the feed of the directory
> +func (d Dir) buildFeed(cfg *Site, feed Feed) ([]byte, error) {
> +	tmpl, ok := cfg.templates.FindTemplate(d.Permalink, feed.Template)
> +	if !ok {
> +		return nil, fmt.Errorf("missing feed template \"%s\" to generate \"%q\"", feed.Template, feed.Title)

There is no need to wrap %s or %q in quotes. Instead, you should use %q
as that wraps the argument in quotes for you.

> +	}
> +
> +	var b bytes.Buffer
> +	data := struct {
> +		Title     string    // Feed title.
> +		Permalink string    // Feed permalink.
> +		Updated   time.Time // Last updated time.
> +		Pages     []*Page   // Feed pages.
> +	}{
> +		Title:     feed.Title,
> +		Permalink: d.Permalink,
> +		Updated:   time.Now(),

Perhaps we should use the build start time instead of the current time?
This could be done later on.

> +		Pages:     d.Pages,
> +	}
> +	if err := tmpl.Execute(&b, data); err != nil {
> +		return nil, err
> +	}
> +	return b.Bytes(), nil
> +}
> +
> +func (d *Dir) addExtraFile(name string, content []byte) {
> +	if d.extraFiles == nil {
> +		d.extraFiles = map[string][]byte{name: content}
> +	} else {
> +		d.extraFiles[name] = content
> +	}
> +}
> +
>  // write writes the directory's contents to the provided destination path.
>  func (d *Dir) write(dstDir string, task *Task) error {
>  	dirPath := pathpkg.Join(dstDir, d.Permalink)
> @@ -255,6 +302,22 @@ func (d *Dir) write(dstDir string, task *Task) error {
>  		}
>  	}
>  
> +	for name, content := range d.extraFiles {
> +		if strings.HasPrefix(name, "/") {
> +			// if name starts with /, put it at the root
> +			name = strings.TrimPrefix(name, "/")
> +		} else {
> +			// otherwise put it under d.Permalink
> +			name = pathpkg.Join(d.Permalink, name)
> +		}

For the sake of simplicity we might want to require an absolute path
anyways.

> +
> +		dstPath := pathpkg.Join(dstDir, name)
> +		os.MkdirAll(dirPath, 0755)
> +		if err := os.WriteFile(dstPath, content, 0644); err != nil {
> +			return err
> +		}
> +	}
> +
>  	// Write subdirectories
>  	for _, dir := range d.Dirs {
>  		dir.write(dstDir, task)
> diff --git a/site.go b/site.go
> index 925ece7..31dd3c2 100644
> --- a/site.go
> +++ b/site.go
> @@ -31,6 +31,14 @@ type Task struct {
>  	StaticDir   string            `toml:"static_dir"`  // static file directory
>  	OutputDir   string            `toml:"output_dir"`  // output directory
>  	UglyURLs    bool              `toml:"ugly_urls"`   // whether to use ugly URLs
> +	Feeds       []Feed            `toml:"feeds"`

One potential improvement here would be to build a map of feeds like we
do for permalinks so that we don't have to loop through them each time
we generate a directory. If you're not comfortable doing this I can
handle it.

> +}
> +
> +type Feed struct {
> +	InputDir string `toml:"input_dir"`
> +	Title    string `toml:"title"`
> +	Template string `toml:"template"`
> +	Output   string `toml:"output"`
>  }
>  
>  func (t *Task) Match(ext string) bool {
> diff --git a/templates/_default/atom.xml b/templates/_default/atom.xml
> index 89cdd2a..553da4b 100644
> --- a/templates/_default/atom.xml
> +++ b/templates/_default/atom.xml
> @@ -4,7 +4,7 @@
>  <title>{{ .Title }}</title>
>  <updated>{{ .Updated.Format "2006-01-02T15:04:05Z07:00" }}</updated>
>  <link href="{{ index site.URLs 0 | safeURL }}{{ .Permalink }}" rel="alternate"/>
> -{{ range .Entries }}<entry>
> +{{ range .Pages }}<entry>

This is a breaking change. I'm fine with making this change, but in that
case we should also remove support for the old [feeds] configuration.

>  	<id>{{ index site.URLs 0 }}{{ .Permalink }}</id>
>  	<title>{{ .Title }}</title>
>  	<updated>{{ .Date.Format "2006-01-02T15:04:05Z07:00" }}</updated>
> -- 
> 2.33.0
Details
Message ID
<0078b8c3-b6a7-4c62-885e-56db1f0bb5af@www.fastmail.com>
In-Reply-To
<CDXUV3LR2PB9.22II9NO3F87ZO@nitro> (view parent)
DKIM signature
missing
Download raw message
Thanks for the review.

Let me know how I should handle the following points, so that I can make a patch v2:
- feed => integrate inside feeds?
- feeds.Permalink or Dir.path?
(see the details below)

> > +	extraFiles map[string][]byte // extra generated files
> 
> Perhaps this field should be named 'feeds' to make it clear what purpose
> it serves. Eventually we'll also want to remove the current 'feed'
> field. I don't think we'll be needing to store any other extra files any
> time soon.
> 

I started with `feeds`, but then I realized that it could be any file in there, hence the renaming to `extraFiles`.
I will switch back to `feeds`.

Should I remove the `feed` field and integrate it inside `feeds`?

> > +		if strings.Trim(d.Permalink, "/") != strings.Trim(feed.InputDir, "/") {
> 
> I think it might be better to require an exact match with the directory
> permalink, as that is how the configuration for [permalinks] and the
> (soon to be deprecated) [feeds] works. Alternatively, we could bring
> those in line with this behavior.
> 

I agree, that my code looks quite hacky.
Another option would be to add a `path` field to the `Dir` struct and
compare its value.
I actually don't really like using Permalink here, since it is not changeable
at the config level (the Permalink of a Dir will always be the folder name).
In contrast to the permalink of a page, which can be changed in the config.


> > +		return nil, fmt.Errorf("missing feed template \"%s\" to generate \"%q\"", feed.Template, feed.Title)
> 
> There is no need to wrap %s or %q in quotes. Instead, you should use %q
> as that wraps the argument in quotes for you.
> 

Good catch, I will fix this.

> > +		Updated:   time.Now(),
> 
> Perhaps we should use the build start time instead of the current time?
> This could be done later on.
> 

Yes, this is probably out of scope for this patch.

> > +	for name, content := range d.extraFiles {
> > +		if strings.HasPrefix(name, "/") {
> > +			// if name starts with /, put it at the root
> > +			name = strings.TrimPrefix(name, "/")
> > +		} else {
> > +			// otherwise put it under d.Permalink
> > +			name = pathpkg.Join(d.Permalink, name)
> > +		}
> 
> For the sake of simplicity we might want to require an absolute path
> anyways.
> 

Alright, so simply `dstPath := pathpkg.Join(dstDir, name)`.
I will change it.

> > +	Feeds       []Feed            `toml:"feeds"`
> 
> One potential improvement here would be to build a map of feeds like we
> do for permalinks so that we don't have to loop through them each time
> we generate a directory. If you're not comfortable doing this I can
> handle it.
> 

It could be an option, but if the map key is the input folder (or permalink), 
then the value must be a slice of feeds (if I want a RSS and an atom feed
for a given folder).

Since the number of feeds (and number of folders per taks) will likely be quite low, 
I don't think that this will bring any significant improvement on speed or readability
(maybe a benchmark will prove me wrong).

> > +{{ range .Pages }}<entry>
> 
> This is a breaking change. I'm fine with making this change, but in that
> case we should also remove support for the old [feeds] configuration.
> 

This is just an update of the default template, not a breaking change.
.Pages is only there for the feeds configured via [[tasks.feeds]]
Line 199 of dir.go stays unchanged (for [feeds]):

Entries   []*Page   // Feed entries.
Details
Message ID
<CDYZ4SYENAIA.SXK84OZPU2QB@nitro>
In-Reply-To
<0078b8c3-b6a7-4c62-885e-56db1f0bb5af@www.fastmail.com> (view parent)
DKIM signature
pass
Download raw message
On Wed Sep 1, 2021 at 4:03 AM EDT, Olivier C wrote:
> Thanks for the review.
>
> Let me know how I should handle the following points, so that I can make a patch v2:
> - feed => integrate inside feeds?
> - feeds.Permalink or Dir.path?
> (see the details below)
>
> > > +	extraFiles map[string][]byte // extra generated files
> > 
> > Perhaps this field should be named 'feeds' to make it clear what purpose
> > it serves. Eventually we'll also want to remove the current 'feed'
> > field. I don't think we'll be needing to store any other extra files any
> > time soon.
> > 
>
> I started with `feeds`, but then I realized that it could be any file in there, hence the renaming to `extraFiles`.
> I will switch back to `feeds`.

I realize now that the [[task.feeds]] feature can be abused to generate
any file, not just feeds. In this case the name extraFiles should be
fine. We might even want to rename the [[task.feeds]] config option to
clarify this. I'll see if I can think of a better name later on.

> Should I remove the `feed` field and integrate it inside `feeds`?

Not necessary for now.

> > > +		if strings.Trim(d.Permalink, "/") != strings.Trim(feed.InputDir, "/") {
> > 
> > I think it might be better to require an exact match with the directory
> > permalink, as that is how the configuration for [permalinks] and the
> > (soon to be deprecated) [feeds] works. Alternatively, we could bring
> > those in line with this behavior.
> > 
>
> I agree, that my code looks quite hacky.
> Another option would be to add a `path` field to the `Dir` struct and
> compare its value.

I think that we should require a root "/" in paths. However, I'm fine
with allowing the trailing slash to be ommitted. For now we can simply
check if d.Permalink == feed.InputDir.

> I actually don't really like using Permalink here, since it is not changeable
> at the config level (the Permalink of a Dir will always be the folder name).
> In contrast to the permalink of a page, which can be changed in the config.

This is desired behavior. Otherwise, it would be impossible to add pages
to the feed when using a permalink like /YEAR/MONTH/DAY/FILENAME, as the
files would be split across multiple directories.

> > > +	Feeds       []Feed            `toml:"feeds"`
> > 
> > One potential improvement here would be to build a map of feeds like we
> > do for permalinks so that we don't have to loop through them each time
> > we generate a directory. If you're not comfortable doing this I can
> > handle it.
> > 
>
> It could be an option, but if the map key is the input folder (or permalink), 
> then the value must be a slice of feeds (if I want a RSS and an atom feed
> for a given folder).
>
> Since the number of feeds (and number of folders per taks) will likely be quite low, 
> I don't think that this will bring any significant improvement on speed or readability
> (maybe a benchmark will prove me wrong).

Correct. It shouldn't be a significant improvement if the number of
feeds is low. We can ignore this for now.

I wonder if we can simply run feed generation _after_ generating all the
dirs. I'll investigate this later.

> > > +{{ range .Pages }}<entry>
> > 
> > This is a breaking change. I'm fine with making this change, but in that
> > case we should also remove support for the old [feeds] configuration.
> > 
>
> This is just an update of the default template, not a breaking change.
> .Pages is only there for the feeds configured via [[tasks.feeds]]
> Line 199 of dir.go stays unchanged (for [feeds]):
>
> Entries   []*Page   // Feed entries.

Ah, you're right. In that case it should be fine.
Reply to thread Export thread (mbox)