dotandimet / Mojo-Feed

Mojo::DOM based Feed Parser
Other
4 stars 1 forks source link

Extending Mojo::Feed #13

Open dotandimet opened 6 years ago

dotandimet commented 6 years ago

Mojo::Feed and Mojo::Feed::Item both expose a dom object, so it should be simple to extract more specialized fields from these objects. It would be nice to allow a simple extension mechanism for this.

Both Mojo::Feed and Mojo::Feed::Item derive from Mojo::Base, so we can use roles to extend their functionality. I added a simple role example, with a method that returns the type of a feed: https://github.com/dotandimet/Mojo-Feed/blob/master/examples/extending.pl

However, there are complications. To extend Mojo::Feed::item, you need to override items in Mojo::Feed; to smoothly extend Mojo::Feed, you need to modify the internals of Mojo::Feed::Reader.

Or we could make all these classes injectable (ie, Mojo::Feed has item_class => 'Mojo::Feed::Item';, etc)

mdom commented 6 years ago

Hah! That was a issue on my todo list i also wanted to bring up... I like the indirection with the injectible class and i would definitely add that to make subclassing and composing easier. There will always be cases of code that you can't include and we should make that as easy for users as possible.

But said that, why not just add type to Mojo::Feed? I would even go so far and add all attributes mentioned in the spec. That shouldn't be too much work and have a negligent runtime cost. And even if we worry about that, we could just use a specialised import list to generate a minimal set of accessors or an extended list. I would love to use atoms expired to save updating feeds if they run their course. :)

dotandimet commented 6 years ago

There are two points here: The one I'm looking at here is how to make Mojo::Feed and Mojo::Feed::Item extensible. feed_type() is just a quick example, doesn't matter if it's a good idea to add that to Mojo::Feed. For extending Mojo::Feed::Item, I'm using your enclosures code - testing how it would look if you had to add that feature without changing the Mojo::Feed::Item code.

The second point (the one you're making) is what are worthwhile additions to Mojo::Feed. The current implementation is a quick-and-dirty, broadest common denominator parser. But if we go past that, it might be worth taking this more seriously and providing full access to everything feeds contain. I'm adding this as a new issue, #14

Regarding extensibility, I made a quick POC in the extend branch with pluggable classes, and I'm not sure I like that approach - it feels over-engineered rather than elegant. In Israel they say "setup for an air conditioner", because the code looks like a wall with a bricked-up hole in it (I think the more common phrase is YAGNI).

A better(?) approach might be to use with_roles to extend the objects themselves, after they are created. This has the advantage of requiring no adaptations in Mojo::Feed (no item_class member, etc), but the user will need to wrap the creation of objects with a function that calls with_roles on the result.

mdom commented 6 years ago

Yeah, you're probably right. Maybe we just just with_roles, see how often we need to extend Mojo::Feed and how much pain the manual wrapping is? We can always add more features but it's hard to remove them ... :)