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
_______________________________________________
List: [email protected]
Listinfo: http://lists.scsys.co.uk/cgi-bin/mailman/listinfo/catalyst
Searchable archive: http://www.mail-archive.com/[email protected]/
Dev site: http://dev.catalyst.perl.org/
