Make itunes compatible optional #1411
Conversation
in generated xml
|
I think there should be a migration that turns the flag on for existing agents. Otherwise this will break existing feeds that are expected to be the way they are now. |
|
Good point. How would I do that? |
| @@ -156,6 +157,12 @@ def feed_description | |||
| interpolated['template']['description'].presence || "A feed of Events received by the '#{name}' Huginn Agent" | |||
| end | |||
|
|
|||
| def xml_namespace | |||
| if (interpolated['itunes_compatible'] == "true") | |||
There was a problem hiding this comment.
Use our boolify helper to handle true as well.
if boolify(interpolated['itunes_compatible'])
|
Good idea! I agree that a spec and migration are needed for this. The migration could probably be similar to |
|
Sounds good. I'll have a look and try to submit later in the week. |
|
Sorry for taking so long. Hopefully this does the trick? |
| @@ -24,6 +24,8 @@ class DataOutputAgent < Agent | |||
| * `template` - A JSON object representing a mapping between item output keys and incoming event values. Use [Liquid](https://github.com/cantino/huginn/wiki/Formatting-Events-using-Liquid) to format the values. Values of the `link`, `title`, `description` and `icon` keys will be put into the \\<channel\\> section of RSS output. Value of the `self` key will be used as URL for this feed itself, which is useful when you serve it via reverse proxy. The `item` key will be repeated for every Event. The `pubDate` key for each item will have the creation time of the Event unless given. | |||
| * `events_to_show` - The number of events to output in RSS or JSON. (default: `40`) | |||
| * `ttl` - A value for the \\<ttl\\> element in RSS output. (default: `60`) | |||
| * `ns_media` - Add [yahoo media namespace](https://en.wikipedia.org/wiki/Media_RSS) in output xml (default: `true`) | |||
There was a problem hiding this comment.
I'd suggest removing the "default" words here, since it doesn't actually get set to true unless the user includes ns_media and ns_itunes.
There was a problem hiding this comment.
I'd also be happy to make this change and merge-- let me know.
There was a problem hiding this comment.
I already made the change. It's good to go.
|
Other than my one comment, looks great and worked when I tested the migration. |
as per comment
in generated xml to address issue #1408
If this is acceptable, I'll update the rspec files as well