Closed lrusak closed 6 years ago
Thaks for the patch. Can you please make it PEP8/flake8 compliant? See the failing tests.
I updated it. Anything else I can do?
Is this going to be merged at some point? Would be very useful.
Another vote to have this merged.
Finally found some time to do a proper review, sorry for letting you wait. I found some issues for which I wrote patches and pushed them into the branch media-content
. Can you check if these changes are ok for you?
Media RSS is specified mainly for RSS (hence the name) but you only extended the atom XML, meaning no media element would end up in the RSS. which is odd and should not be like this. Fixed by commit 61d1073
Using fe.media.media_content(…)
is quite redundant. I changed the name to
fe.media.content(…)
. The same for the thumbnail method. The commit is
f1eeb48.
An item can have multiple content elements. I have modified the method to
accept an element either as keyword arguments, or as dictionary or multiple
elements as a list of dictionaries. Multiple calls to content()
will just
add additional elements unless the parameter replace
is set. The patch is
696cc4c.
media:content
can be put in media groups or directly into an item. I have
added an option to define a media group for the content, with setting a
default group if nothing is specified. This is part of 696cc4c as well.
Finally, there can be multiple mediia:thumbnail
as well. Commit 2b4b04e
enables this in the same manner as with the content. Note that so far, this
will neither let you set a thumbnail on channel
level, nor on
media:content
level.
Still a lot of the Media RSS elements are not implemented. But that is something to expand for another time.
Just merged my modified version since there was no reply/complaint about it.
Thanks! Apologies that I didn't have time to follow up!
First off, thank you for the wonderful library. I needed to generate an atom feed for a project I am working on and this library fit the bill. It was missing the media tags I needed so I decided to add them.
This doesn't include all the media tags, it just includes the ones I need but it is a good start.
I don't really want to add more than this but maybe you'd like to include this as a good start to this extension (I didn't really want to do the complete implementation or write tests).
I followed this, http://www.rssboard.org/media-rss#media-content