Skip to content

Conversation

@bhagany
Copy link
Contributor

@bhagany bhagany commented Dec 1, 2016

Following @MartyGentillon, here: #109 (comment). Intent is more clear, and this change or something like it is a prerequisite for fulfilling the goals of #109.

@martinklepsch
Copy link
Contributor

martinklepsch commented Dec 1, 2016

Two questions:

  • Can you think of situations where you'd want to render one atom-feed and another rss-feed with different items?
  • This somehow couples the content task to the implementation of the RSS and Atom tasks as these know about those keys and read from them. Maybe the default-meta map should be supplied as an option and thus be extendable to the users needs? (We could still have some sensible defaults ofc)

@bhagany
Copy link
Contributor Author

bhagany commented Dec 1, 2016

  • No, I just went for maximum flexibility. I can't even think of a situation where you're using both atom-feed and rss. In any case, this implementation is just as coupled as before, it's just that in this case the coupling goes the opposite way. (in master, atom-feed and rss know about markdown, in this branch, markdown knows about atom-feed and rss)
  • Yes! I have a pull request planned to modify the meta in exactly the way you describe. In the meantime, this implementation allows for overriding the default by putting it in the yaml in markdown files, or the original method of changing the :filterer on atom-feed and rss.

@martinklepsch martinklepsch merged commit a0801ea into hashobject:master Dec 1, 2016
@martinklepsch
Copy link
Contributor

Ok sounds good to me then 👍

I'll just go ahead and merge @Deraen @podviaznikov

@bhagany bhagany deleted the explicit-feeds branch December 1, 2016 05:59
@bhagany
Copy link
Contributor Author

bhagany commented Dec 1, 2016

Great, thanks!

@podviaznikov
Copy link
Member

This looks good to me. Great change.
We only need to update our Spec file https://github.com/hashobject/perun/blob/master/SPEC.md with those new properties.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants