frioux / DBIx-Class-Helpers

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

Reduce inheritance, fix #91 #103

Closed dakkar closed 4 years ago

dakkar commented 4 years ago

I have removed most use parent, and replaced the necessary ones with the smallest needed class.

I've also added a test showing the problem reported in issue #91. I think the root of the problem is that derived classes that don't call load_components don't get the C3 mro.

All tests still pass on my machine.

dakkar commented 4 years ago

Yep, confirmed, adding use mro 'c3' to the derived classes in t/component-order.t makes it pass without my patches. I still think that it's a good idea to reduce the inheritance tree, though.

coveralls commented 4 years ago

Coverage Status

Coverage decreased (-0.005%) to 98.557% when pulling a171707681160972e94009f8b17476466c00977c on dakkar:reduce-inheritance into 3ce57a3da053ede77877d40c954fa11aabdb4eff on frioux:master.

frioux commented 4 years ago

Unless you can show a concrete improvement from this patchset I'm inclined to close it. This is what DBIx::Class::Helper::Schema::Verifier::C3 was built to automate: people must use c3, one way or another, with DBIC.

dakkar commented 4 years ago

people must use c3, one way or another, with DBIC

Very fair! Maybe add a line of documentation in https://github.com/frioux/DBIx-Class-Helpers/blob/master/lib/DBIx/Class/Helper/ResultSet.pm and to a Result helper or doc-only package, explaining that?

One of the problems is that Schema::Loader puts a use base in the generated Result classes, and no use mro 'c3', even whed a custom base class is specified (that's how I encountered this issue). Should I ask the Schema::Loader maintaner to add that?

frioux commented 4 years ago

yeah they either need to use mro 'c3' or I think calling load_components does that for you as well.

dakkar commented 4 years ago

I reported the suggestion to Schema::Loader https://rt.cpan.org/Ticket/Display.html?id=132035

frioux commented 4 years ago

Cool. Gonna close this one now. Thanks!