find-all matching adjusted

[cmstrickland]

I want to be able to add differently rendered/themes post 'sub-types' to my blog ( linkblog entries, photo galleries, journal entries etc. ). To this end, I'm experimenting with a plugin that allows me to subclass existing document types, to add additional properties/slots that can be handled conditionally. I ran into a small snag, in as much as the current implementation of find-all is too greedy wrt matching on type of document.

The existing implementation of find-all implementation returns false positives for subclasses I added a parameter of a predicate to implement matching, with a default value that preserves original behaviour

I then added an exact class matcher to purge-all invocation of find-all to stop it purging subclasses

[kingcons]

Thanks for the pull request! That's good functionality and I'm open to it. I'd love to see one of your sub-type plugins, if possible.

A few thoughts:

1) I'm not wild about this implementation as it relies on comparing class names. I did this, admittedly, as part of the incremental plugin[0] but it remains a kludge.

At the very least it would be nice to see a class-name-p function factored into util.lisp and reused both in the incremental plugin and in your purge-all call. I'm also a little nervous that this is becoming the default purge-all functionality.

That said, I don't think we relied on the implicitly purging subclasses behavior previously. I'll have to double check that before merging just to be sure. This is exactly why I should find some time to write a good test suite. But that time has been hard to come by of late. :-/

2) Just a small nit: For find-all's optional parameter I'd prefer the name matcher to matching or even matches-p since it should be a predicate. :)

[cmstrickland]

On 3 Jan 2015, at 20:02, Brit Butler wrote:

Thanks for the pull request! That's good functionality and I'm open to it. I'd love to see one of your sub-type plugins, if possible.

OK. I have a very early working plugin that overloads 'post' to include a 'type' attribute, sitting on a branch. Still WIP. When I get it into an attractive and viable shape, I will submit a PR, but I'm still sketching really. Feel free to have a look, feedback.

https://github.com/cmstrickland/coleslaw/blob/add-post-types/plugins/subposts.lisp

A few thoughts:

1) I'm not wild about this implementation as it relies on comparing class names. I did this, admittedly, as part of the incremental plugin[0] but it remains a kludge.

[0]: https://github.com/redline6561/coleslaw/blob/master/plugins/incremental.lisp#L62

OK, I will think about that further. I couldn't think of any other way to do exact class matching, but I really know very little about CLOS. Perhaps there is something in the MOP that is more accurate. I am happy learning more about this, which is sort of why I'm hacking in this area already.

At the very least it would be nice to see a class-name-p function factored into util.lisp and reused both in the incremental plugin and in your purge-all call. I'm also a little nervous that this is becoming the default purge-all functionality.

Sure, I am happy to do this. I have some unpushed extra work in util that follows on from this already, to do more lenient slot / metadata validating / error signalling on construction, which I think will be necessary if I'm going to start varying the possible metadata headers. I'm lining up a PR for that at the moment.

That said, I don't think we relied on the implicitly purging subclasses behavior previously. I'll have to double check that before merging just to be sure. This is exactly why I should find some time to write a good test suite. But that time has been hard to come by of late. :-/

I'm fairly sure that it doesn't rely on that behaviour, based on a lot of interactive testing/tracing, but yes. My #1 goal is to try and not break existing behaviour, I'm hoping to keep as much custom handling in plugins as possible - it seems like purge-all removing subclasses is a mistake though, even if my patches are heading up the wrong alley - the principal of subclassing other content types seems inherently logical as an extension mechanism, and as soon as you do this, purging is too eager in it's current form.

I hear you about time, this is just my 'hack on the train commute to work' project, so ~ 1 hour a day in a good week :-). FWIW if there were tests, I'd be happy to be contributing to them, but I wouldn't want to start a test framework on my own, at least not until I've fully ported my blog across.

2) Just a small nit: For find-all's optional parameter I'd prefer the name matcher to matching or even matches-p since it should be a predicate. :)

Sure thing.

Regards, Colin M. Strickland, cms, 'that guy'.

[kingcons]

HEY! Sorry I haven't gotten a chance to look at this in detail yet. This has been my first week as a Ruby Instructor at a 12-week intensive code school called the Iron Yard. Will have a look this weekend. :)

[dertuxmalwieder]

I did.