frioux / DBIx-Class-Helpers

https://metacpan.org/pod/DBIx::Class::Helpers
20 stars 38 forks source link

Make all helpers verify that they got composed into the right type (Result, Set, Schema) of class #62

Closed frioux closed 3 years ago

frioux commented 8 years ago
08:46:05 ribasushi | <ribasushi> frew: ^^ (last ~10ish lines) perhaps a C::C3::C post-apply hook check that
                   | someone tried to add a resultset helper to a result, or something....?
08:46:29      frew | right I saw that but I don't see why you think that?
08:47:13      frew | like, he posted a thing where he did something stupid, and it has a stacktrace that is a
                   | little confusing but it's hard when someone does something crazy like use an RS component in
                   | a result
08:47:26 ribasushi | because when people end up doing this, the failure ( site / exception text ) is completely
                   | impenetrable unless you know what to look for
08:47:32      frew | ooohhh
08:47:41 ribasushi | mainly because add_columns clashes (both in resultsource proxy and in your helper )
08:47:42      frew | you are recommending a possible additional helper base to check
08:47:44      frew | I hear you
08:48:03 ribasushi | so the very next thing after load_components is CXSA barfing because it is called on a class
08:48:10      frew | right I see
08:48:14 ribasushi | yes!
08:48:14 ribasushi | :)
08:48:33      frew | I'll add it to the list
ribasushi commented 8 years ago

Thinking about this a bit (since I am in that headspace) and wanted to jolt down my extra thoughts before swapping the problem out.

This kind of check can not be really done with a helper, nor with my sanitycheck framework. Or more specifically - the incomprehensible exception will be encountered way before anything else had a chance to run. The example below is not hypothetical - the original problem that sparked the conversation was exactly this:

~$ perl -e '
  use base "DBIx::Class::Core";

  __PACKAGE__->load_components("Helper::ResultSet::Shortcut::AddColumns");

  # it does not matter we never called table() - we will never reach
  # the point where the lack of result_source_instance is fatal
  __PACKAGE__->add_column("boo")
'
Class::XSAccessor: invalid instance method invocant: no hash ref supplied at /home/rabbit/perl5/perlbrew/perls/5.16.2/lib/site_perl/5.16.2/DBIx/Class/ResultSet.pm line 368.

My original suggestion of specific helpers with clashing method names (like ::AddColumns) adopting an apply hook, is still the only way I can think of solving this.

frioux commented 8 years ago

When the verification framework that's written for dbic is released I'll close this, and deprecate the overlapping helpers that already exist.

sent from a rotary phone, pardon my brevity

On Jun 14, 2016 10:47 PM, "Peter Rabbitson" notifications@github.com wrote:

Thinking about this a bit (since I am in that headspace) and wanted to jolt down my extra thoughts before swapping the problem out.

This kind of check can not be really done with a helper, nor with my sanitycheck framework. Or more specifically - the incomprehensible exception will be encountered way before anything else had a chance to run. The example below is not hypothetical - the original problem that sparked the conversation was exactly this:

~$ perl -e ' use base "DBIx::Class::Core";

PACKAGE->load_components("Helper::ResultSet::Shortcut::AddColumns");

it does not matter we never called table() - we will never reach

the point where the lack of result_source_instance is fatal

PACKAGE->add_column("boo") ' Class::XSAccessor: invalid instance method invocant: no hash ref supplied at /home/rabbit/perl5/perlbrew/perls/5.16.2/lib/site_perl/5.16.2/DBIx/Class/ResultSet.pm line 368.

My original suggestion of specific helpers with clashing method names (like ::AddColumns) adopting an apply hook https://metacpan.org/pod/Class::C3::Componentised::ApplyHooks#before_apply, is still the only way I can think of solving this.

— You are receiving this because you authored the thread. Reply to this email directly, view it on GitHub https://github.com/frioux/DBIx-Class-Helpers/issues/62#issuecomment-226093840, or mute the thread https://github.com/notifications/unsubscribe/AAAf47mPp4n1_exSSFHvImt3119fOCqgks5qL5HlgaJpZM4Hmo_f .

ribasushi commented 8 years ago

Errrr note that I explicitly said earlier:

This kind of check can not be really done with a helper, nor with my sanitycheck framework.

frioux commented 8 years ago

I know; still worth leaving open in case we all die and someone needs to see where it's at :P

On Mon, Aug 22, 2016 at 10:13 PM, Peter Rabbitson notifications@github.com wrote:

Errrr note that I explicitly said earlier:

This kind of check can not be really done with a helper, nor with my sanitycheck framework.

— You are receiving this because you authored the thread. Reply to this email directly, view it on GitHub https://github.com/frioux/DBIx-Class-Helpers/issues/62#issuecomment-241628830, or mute the thread https://github.com/notifications/unsubscribe-auth/AAAf48qxl4OZTvV_VAiBPrp3fOExSCMdks5qioGIgaJpZM4Hmo_f .

fREW Schmidt http://blog.afoolishmanifesto.com

ribasushi commented 8 years ago

Oh I agree. What I got confused about was:

When the verification framework that's written for dbic is released I'll close this, and deprecate the overlapping helpers that already exist.

... given the originally reported problem was about a "too early to check for" conflict. It seemed that focus got lost, so I wrote back ;)

ribasushi commented 8 years ago

Or in other words: the subject of this ticket needs changing (based on this failcase) to avoid future confusion.