[Catalyst] YA CRUD module

[Tomas Doran]

On 31 Oct 2011, at 14:49, David Schmidt wrote:

> It's about time I publish my first CPAN module and it happens to be
> yet another CRUD module.
>
> Feedback greatly appreciated.


I haven't looked at this in depth - but it generally looks nice (other  
than lack of tests).. But some comments:

You don't need strict or warnings. MX::MethodAttributes::Role gives  
you all the same stuff Moose::Role does, which includes strict and  
warnings.

It might be nice if you could split the roles up..

So there would be a (very base) role, and then separate List/Show/Edit/ 
Create/Delete roles, and the main role would just compose everything...

But that would allow you to more easily / naturally only use a subset  
of the functionality, whilst implementing some of the code in a more  
custom manor.. Of course you can do this by adding around method  
modifiers to methods - however having several classes with half a  
dozen modifiers each which redirect to a 404 (to remove edit / delete  
functionality) is a lot less nice than only composing a subset of the  
roles (if for no other reason than that the wrapped things will show  
up in the debug table)..

The documentation on parent_key doesn't quite seem to make sense, and  
the predicate naming there is also not clear.. (It may make sense why  
it isn't called what you expect - but could do with a comment?)

form_class could be made a MooseX::Types::LoadableClass to avoid the  
explicit class load.

I intensely dislike your sub _redirect - couldn't you instead compute  
a private path, and pass that to $c->uri_for_action to produce a  
'proper' URI?

This would enable people to use your resources at sub-path parts, as  
you could pass the captures in.

e.g.

/user/<id>
/user/<id>/.... resources from your role
/user/<id>/document/<id>
/user/<id>/document/<id>/.... resources from your role

In the latter case, you could / would chain off the base_with_id  
action for the user id... This is the part where it becomes 'fun', and  
where your 'parent' functionality comes in - I guess these two things  
could be nicely combined... :)

Is it sensible to give it an 'index' action? I'd be tempted to rename  
this to 'list', so that if someone wanted to override the default  
index, they could just override the PathPart of the 'list' action, and  
provide their own index action..

Your delete works with GET requests. This is a horribly bad idea as it  
makes GETs stop being idempotent... Same deal with edit/create I  
guess...

I look forward to seeing a TestApp - this code looks like a great  
start on something really interesting :)

Cheers
t0m