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

Extending Mojo::Feed #13

Open
dotandimet opened this issue Apr 4, 2018 · 3 comments
Open

Extending Mojo::Feed #13

dotandimet opened this issue Apr 4, 2018 · 3 comments

Comments

@dotandimet
Copy link
Owner

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
Copy link
Contributor

mdom commented Apr 4, 2018

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
Copy link
Owner Author

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
Copy link
Contributor

mdom commented Apr 11, 2018

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 ... :)

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

No branches or pull requests

2 participants