Matt S Trout escribió:
First, please delete the current copy from CPAN, it's in the wrong namespace.
Column inflators go in DBIx::Class::InflateColumn, not ::Inflator:: - stealing
random DBIC subnamespaces when there's already an accepted convention isn't
very polite and perhaps more importantly will make your code much harder to
find on CPAN.
No problem. Sorry for the namespace invasion. I've already scheduled the
deletion in PAUSE.
Second, the architecture's silly. What you wanted to write was a single
DBIx::Class::InflateColumn::Serializer that uses a driver model like
DBIx::Class::EncodedColumn and DBIx::Class::UUIDColumns do.
+1. Since I started out with a JSON inflator for one of my projects, I
extended the behaviour to other serializers, thinking of usefulness for
other people (not so much in elegance of the code :p).
Third, your code is full of things like -
if (defined $info->{'size'}){
$freezer = get_bounded_column_freezer($info->{'size'});
} else {
$freezer = \&unbounded_freezer;
}
which is catastrophically bad since you've now added methods to the class
that can't actually be called as methods because they're really subs, and
can't be overriden by subclasses either.
Let's see if you like it better like this:
method select_freezer
- selects the get_xxx_freezer to call depending on the properties of
the column (provided just in case someone wants to override the method
for selecting freezers).
- get_bounded_column_freezer and unbounded_freezer get transformed
into $self->get_bounded_column_freezer, and $self->unbounded_freezer.
Both will return subs that let the column get unfrozen. Are you OK with
returning subs?
Within OO code, please only use method calls and document them so people
can override them - otherwise your code isn't usefully pre-packaged because
people will have to copy and paste it in order to extend it ...
Didn't want to document them, since I consider they are internal to the
working of the inflators. maybe I should change them to sub
_get_xxx_freezer? Maybe document select_freezer so people can override
that one?
Hope you like it. I'd love to get feedback from the DBIx community to
get this package to a 1.00 state.
There's no such thing as "the DBIx community". DBIx is the namespace for
DBI extensions, DBIx::Class is one project in that namespace.
s/DBIx/DBIC/ (fast typing. null review).
All that said, I think the -idea- is great - just the name, architecture
and implementation need some work :)
That's what I was looking for... 0.01 exposes an idea. The feedback gets
done what the community wants/needs/thinks (not just what I
want/need/think :))
I look forward to looking at a tarball or repository of a rewritten version
so we can try and give feedback on the new one -before- it goes to CPAN ...
(also, I'd be happy to see this as a "DBIC team supported project" - in
which case could we offer you subversion or git hosting for it? :)
+1 for a SVN repo.
Thanks for the opinion and the pointers,
Jose Luis Martinez
[email protected]
_______________________________________________
List: http://lists.scsys.co.uk/cgi-bin/mailman/listinfo/dbix-class
IRC: irc.perl.org#dbix-class
SVN: http://dev.catalyst.perl.org/repos/bast/DBIx-Class/
Searchable Archive: http://www.grokbase.com/group/[email protected]
