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
[hidden email]
_______________________________________________
List:
http://lists.scsys.co.uk/cgi-bin/mailman/listinfo/dbix-classIRC: irc.perl.org#dbix-class
SVN:
http://dev.catalyst.perl.org/repos/bast/DBIx-Class/Searchable Archive:
http://www.grokbase.com/group/dbix-class@...
