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

Categories and collections #228

Merged
merged 9 commits into from
Sep 6, 2018
Merged

Categories and collections #228

merged 9 commits into from
Sep 6, 2018

Conversation

benbalter
Copy link
Contributor

This pull request builds on @mortenson's work in #176 to fix #70 by abstracting some of the existing logic out a bit. Specifically, it supports generating feeds for categories (and collections other than posts).

To generate feeds for categories:

feed:
  categories:
    - news
    - updates

And to generate feeds for collections:

feed:
  collections:
    - changes

Since posts are a collection, it's also possible to generate category feeds for other collections:

feed:
  collections:
    changes:
      categories:
        - news
        - updates

Everything is backwards compatible with the existing configuration flags, such that:

feed:
  path: "foo.xml"

Automatically gets converted to:

feed:
  collections:
    posts: 
      path: "foo.xml"

So this should be a non-breaking change, and theoretically add as little complexity as necessary to the publishing experience (in most cases, just creating a top-level list in the config to make it opt-in).

By default, categories are in the form of /feed/<category>.xml, collections are /feed/<collection>.xml, and collection categories are /feed/<collection>/<category>/xml.

All this is implemented with a small abstraction in the generator, which passes the collection (defaulting to posts) and optionally the category to the existing template.

Copy link
Member

@DirtyF DirtyF left a comment

Choose a reason for hiding this comment

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

This is ✨ great ✨ work as usual @benbalter 😻

@DirtyF DirtyF requested a review from pathawks May 8, 2018 16:50
@DirtyF DirtyF added the feature label May 8, 2018
@mortenson
Copy link

Thanks for pushing this forward @benbalter !

Copy link
Member

@parkr parkr left a comment

Choose a reason for hiding this comment

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

LGTM! Too bad we can't incorporate collection settings with feed settings, since we're basically duplicating that:

collections:
  changes:
    feed_path: "/feed/ch-ch-ch-changes.xslt"

prefix = collection == "posts" ? "/feed" : "feed/#{collection}"
if category
"#{prefix}/#{category}.xml"
elsif collections[collection]["path"]
Copy link
Member

Choose a reason for hiding this comment

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

We should do nil checking here in case collections is nil or collections[collection] is nil

@benbalter
Copy link
Contributor Author

benbalter commented May 8, 2018

Too bad we can't incorporate collection settings with feed settings, since we're basically duplicating that

The settings for this were the toughest part. We could do, e.g.,:

collections:
  puppies:
    feed: true

or

collections:
  puppies:
    feed: 
      path: foo.xml
      categories:
        - foo

But that felt more complex and it would burry feed settings in collection settings, rather than following the pattern of a top-level key named after the plugin.

Could see it either way (and could always do both, although that feels SUPER complicated to explain the three ways you could configure it).

Unless anyone is strongly opinionated about the config, it sounds like this can merge with a bit more time for feedback?

Edit: Just saw @DirtyF requested @pathawks review.

@pathawks
Copy link
Member

pathawks commented May 9, 2018

I would like to review this. Busy at the moment; please just give me a day or so 🙂

- changes
```

By default, collection feeds will be outputted to `/feed/<COLLECTION>.xml`. If you'd like to customize the output path, specify a collection's custom path as follows:
Copy link
Member

Choose a reason for hiding this comment

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

IMHO ..will be output to reads better..

Choose a reason for hiding this comment

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

Or active voice: "The default output-path for collection feeds is /feed/<COLLECTION>.xml"

lib/jekyll-feed/generator.rb Show resolved Hide resolved
# WIll return `/feed/collection.xml` for other collections
# Will return `/feed/collection/category.xml` for other collection categories
def feed_path(collection: "posts", category: nil)
prefix = collection == "posts" ? "/feed" : "feed/#{collection}"
Copy link
Member

Choose a reason for hiding this comment

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

why is there a leading slash for /feed.xml but not for feed/#{collection}.xml .. also, the comments above actually states the opposite..

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Good catch. That is an oversight. Will push up a fix.

expect(contents).to match /http:\/\/example\.org\/bass\/2014\/03\/04\/march-the-fourth\.html/
expect(contents).to match /http:\/\/example\.org\/bass\/2014\/03\/02\/march-the-second\.html/
expect(contents).to match /http:\/\/example\.org\/bass\/2013\/12\/12\/dec-the-second\.html/
expect(contents).to match /http:\/\/example\.org\/bass\/updates\/2014\/03\/04\/march-the-fourth\.html/
Copy link
Member

Choose a reason for hiding this comment

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

not related to this PR.. but IMO enforcing the percent_r literal on the test files will avoid the need to escape the slashes here..

/cc @DirtyF

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yeah. Given the time, I'd also love to add unit tests to the generator and meta tag (rather than just relying on integration tests).

@halirutan
Copy link

That is great! When can we expect this getting merged?

@mattmcmanus
Copy link

I hate to be annoying, but I would LOVE to be able to use this feature. It looks like this PR has several approvals. Who's responsible to handle the merge and release? Is there anything a non-maintainer can do to help out here?

@DirtyF
Copy link
Member

DirtyF commented Sep 6, 2018

@jekyllbot: :shipit: +minor

@jekyllbot jekyllbot merged commit b85ef9a into master Sep 6, 2018
@jekyllbot jekyllbot deleted the categories-and-collections branch September 6, 2018 18:50
jekyllbot added a commit that referenced this pull request Sep 6, 2018
@DirtyF
Copy link
Member

DirtyF commented Sep 6, 2018

Thanks @mattmcmanus for the reminder, feel free to use the current master branch if you wanna test the feature before we release a proper gem

@mattmcmanus
Copy link

Yeah!

@DirtyF
Copy link
Member

DirtyF commented Sep 6, 2018

works like a charm, expect a release soon: https://frank.taillandier.me/feed/agile.xml

@halirutan
Copy link

I second this. A big YEAH!

meme

@dpocock
Copy link

dpocock commented Apr 18, 2019

I've tried to use this on a Fedora system and it doesn't appear to be working:

$ rpm -qa | grep jekyll
rubygem-jekyll-feed-0.12.1-1.fc29.noarch
rubygem-jekyll-3.8.5-1.fc29.noarch
rubygem-jekyll-watch-2.2.1-1.fc29.noarch
rubygem-jekyll-sass-converter-1.5.2-2.fc29.noarch
rubygem-jekyll-seo-tag-2.6.0-1.fc29.noarch

I tried all the different permutations for category and categories

Here is a snippet of my _config.yml

plugins_dir:
  - jekyll-feed

feed:
  categories:
    - fedora
...

and from one of the blogs _posts/2019-04-07-sfk-2019-prishtina-toastmasters.html

...
category: fedora
...

and I also tried:

...
category: ['fedora']
...

A category-specific feed XML simply doesn't appear:

$ jekyll build
$ !fi
find . -name '*.xml'
./_site/feed.xml
./feed.xml

How should somebody troubleshoot in a situation like this?

@ellenwyllie
Copy link

This isn't working for me either. I added

feed:
  categories:
    - jakartaee

to my _config.yml and category: jakartaee to my blog post's front matter.

The feed for the category is being created, but it doesn't have any posts in it.

I've tried variations of category in the post, such as category: ['jakartaee'], categories: jakartaee and categories: ['jakartaee'] but none of them seem to work.

Does anyone know why this isn't working?

@DirtyF
Copy link
Member

DirtyF commented May 10, 2019

@ellenwyllie : category: jakartaee should work.

@steven1046
Copy link

Does jekyll-feed rely on a certain version of jekyll-assets to work properly?

@mortenson
Copy link

@DirtyF categories: is what's documented in README.md, so I would expect it to work.

@marco-c
Copy link

marco-c commented May 11, 2019

See also #246.

@jekyll jekyll locked and limited conversation to collaborators May 10, 2020
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Option to have feeds by tags/categories