Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Feature/cdbi implicit inflate #51

Conversation

ungrim97
Copy link
Contributor

Class::DBI allows the foreign_class to be a non database class. e.g DateTime.

Inflation and deflation can be controlled either by supplying:

PACKAGE->has_a(accessor => 'Foreign::Class', inflate => sub {}, deflate => sub {});

however if inflate isn't supplied then CDBI will call: Foreign::Class->new($val) and if deflate is not supplied it will stringify "$val".

This behaviour is mostly used for inflation and deflation of datetime columns and should be emulated

@ungrim97 ungrim97 closed this Jul 18, 2014
@ungrim97 ungrim97 reopened this Jul 18, 2014
@ungrim97
Copy link
Contributor Author

15:24 < mrf> ribasushi: pull request sent
15:33 < ribasushi> mrf: several nits
15:34 < ribasushi> I srtongly prefer the tests and implmentation in one commit - allows for proper bisecting
15:34 < ribasushi> you can not assume DateTime is available - grep for "Optional::Dependencies" to see how to use it to properly skip the test when the thing isn't there
15:35 < ribasushi> please move Date.pm into testlib, and give it a little less generic name
15:36 < ribasushi> the implementation itself - you are subtly changing the if() semantics - previously if there was and inflate/deflate it would do stuff regardless of object type (even on db-stuff) - now this is no longer the case
15:37 < ribasushi> (I am reading the diff under -w, makes this much more obvious)
15:38 < ribasushi> mrf: other than that - excellent stuff, please see the above comments and resubmit the pr
15:38 < ribasushi> mrf++
15:38 < mrf> no probs will impliment and chuck back.
15:38 < mrf> do you want me to squash it down into one commit.
15:38 < mrf> ?
15:38 < ribasushi> correct
15:39 < ribasushi> mrf: the rationale is: bashing-into-submission history is not valuable enough to make the life of future bisecters/blamers difficult

@ribasushi
Copy link
Collaborator

This has now been merged at 39e2a212 with the seemingly more correct change:

   # Class::DBI allows Non database has_a with implicit deflate and inflate
   # Hopefully the following will catch Non-database tables.
-  if (!$f_class->isa('DBIx::Class') && !$f_class->isa('Class::DBI')){
-    $args{'inflate'} ||= sub { $f_class->new(shift); }; # implicite inflate by calling new
-    $args{'deflate'} ||= sub { my $value = shift; "$value"; }; # implicite deflate by stringification
+  if( !$f_class->isa('DBIx::Class::Row') and !$f_class->isa('Class::DBI::Row') ) {
+    $args{'inflate'} ||= sub { $f_class->new(shift) }; # implicit inflate by calling new
+    $args{'deflate'} ||= sub { shift . '' }; # implicit deflate by stringification
   }

Please let me know if this is in fact wrong (perhaps you do want to exempt ::ResultSet instances just like ::Row... though it doesn't make much sense)

@ribasushi ribasushi closed this Jul 24, 2014
@ungrim97 ungrim97 deleted the feature/cdbi_implicit_inflate branch July 24, 2014 07:32
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
2 participants