Bug #82319 for Module-Implementation: [PATCH] Minimize dependency footprint of Module::Implementation

Sat Dec 29 00:47:37 2012 ribasushi [...] leporine.io - Ticket created

Subject:

Minimize dependency footprint of Module::Implementation

A question was raised at [1] wrt suitability of Module::Implementation as a dependency for a "low level" module. It got me curious and the result is a set of non-invasive patches leaving M::I with only one non-core dependency - Module::Runtime, which in turn has a commitment of zero-depencencies. Please let me know if the 3 patches here [2] work for you. If you have concerns about some of the changes - please let me know so I can rectify those. Cheers [1] https://rt.cpan.org/Ticket/Display.html?id=78272#txn-1160095 [2] https://github.com/ribasushi/module_implementation/commits/master

Sat Dec 29 00:47:57 2012 ribasushi [...] leporine.io - Subject changed from 'Minimize dependency footprint of Module::Implementation' to '[PATCH] Minimize dependency footprint of Module::Implementation'

Sun Dec 30 11:35:43 2012 autarch [...] urth.org - Correspondence added

Subject:	Re: [rt.cpan.org #82319] Minimize dependency footprint of Module::Implementation
Date:	Sun, 30 Dec 2012 10:35:28 -0600 (CST)
To:	Peter Rabbitson via RT <bug-Module-Implementation [...] rt.cpan.org>
From:	Dave Rolsky <autarch [...] urth.org>

On Sat, 29 Dec 2012, Peter Rabbitson via RT wrote: Show quoted text

> A question was raised at [1] wrt suitability of Module::Implementation > as a dependency for a "low level" module. It got me curious and the > result is a set of non-invasive patches leaving M::I with only one > non-core dependency - Module::Runtime, which in turn has a commitment of > zero-depencencies. > > Please let me know if the 3 patches here [2] work for you. If you have > concerns about some of the changes - please let me know so I can rectify > those.

Haven't we gone down this road before? What's the use case for removing all these deps? I _like_ using them. It makes writing and maintaining code easier for me. -dave /*============================================================ http://VegGuide.org http://blog.urth.org Your guide to all that's veg House Absolute(ly Pointless) ============================================================*/

Sun Dec 30 11:35:44 2012 The RT System itself - Status changed from 'new' to 'open'

Sun Dec 30 11:52:32 2012 ribasushi [...] leporine.io - Correspondence added

Subject:	Re: [rt.cpan.org #82319] Minimize dependency footprint of Module::Implementation
Date:	Mon, 31 Dec 2012 03:52:19 +1100
To:	"autarch [...] urth.org via RT" <bug-Module-Implementation [...] rt.cpan.org>
From:	Peter Rabbitson <ribasushi [...] cpan.org>

On Sun, Dec 30, 2012 at 11:35:44AM -0500, autarch@urth.org via RT wrote: Show quoted text

> What's the use case for removing all these deps?

So that none of these deps (nor their deps) need to think twice before relying on Module::Implementation. The very nature of M::I places it at the "bottom of the foodchain", at a similar level to Module::Runtime. Incidentally I made an identical request to Module::Runtime a year ago: https://rt.cpan.org/Public/Bug/Display.html?id=74805 I am fully aware that we have different priorities wrt the health of the near-core toolchain. Therefore all I can do is submit better code and hope you will include it. If not the case - close the ticket, and we'll rinse/repeat on some other module ;) Cheers

Tue Jan 01 21:01:44 2013 ribasushi [...] leporine.io - Correspondence added

On Sun Dec 30 11:52:32 2012, RIBASUSHI wrote: Show quoted text

> Therefore all I can do is submit better code and > hope you will include it. If not the case - close the ticket

To clarify in case it didn't come across clearly - I would have no qualms about the patch being rejected. However, regardless which way you decide to go, it would be nice to have a definitive answer. For instance if I knew you would be opposed to the idea, I would not have made the recommendation I made in [1]. Being informed helps ;) Cheers [1] https://rt.cpan.org/Public/Bug/Display.html?id=78272#txn-1153215

Tue Jan 01 21:47:18 2013 autarch [...] urth.org - Correspondence added

Subject:	Re: [rt.cpan.org #82319] [PATCH] Minimize dependency footprint of Module::Implementation
Date:	Tue, 1 Jan 2013 20:47:06 -0600 (CST)
To:	Peter Rabbitson via RT <bug-Module-Implementation [...] rt.cpan.org>
From:	Dave Rolsky <autarch [...] urth.org>

On Tue, 1 Jan 2013, Peter Rabbitson via RT wrote: Show quoted text

> Queue: Module-Implementation > Ticket <URL: https://rt.cpan.org/Ticket/Display.html?id=82319 > > > On Sun Dec 30 11:52:32 2012, RIBASUSHI wrote:

>> Therefore all I can do is submit better code and >> hope you will include it. If not the case - close the ticket

> > To clarify in case it didn't come across clearly - I would have no > qualms about the patch being rejected. However, regardless which way you > decide to go, it would be nice to have a definitive answer. For instance > if I knew you would be opposed to the idea, I would not have made the > recommendation I made in [1]. Being informed helps ;)

Is this change necessary for Package::Stash to use M::I? It already has non-core deps, and in fact it uses Test::Fatal, which you're trying to remove from the M::I test suite. The only new dep beyond M::I that would be added to P::S is Try::Tiny, which is already a dependency of hundreds of other modules. So I really don't see the point of removing these dependencies for the sake of P::S, whose author hasn't even asked for these changes. -dave /*============================================================ http://VegGuide.org http://blog.urth.org Your guide to all that's veg House Absolute(ly Pointless) ============================================================*/

Tue Jan 01 21:58:52 2013 ribasushi [...] leporine.io - Correspondence added

On Tue Jan 01 21:47:18 2013, autarch@urth.org wrote: Show quoted text

> On Tue, 1 Jan 2013, Peter Rabbitson via RT wrote: >

> > Queue: Module-Implementation > > Ticket <URL: https://rt.cpan.org/Ticket/Display.html?id=82319 > > > > > On Sun Dec 30 11:52:32 2012, RIBASUSHI wrote:

> >> Therefore all I can do is submit better code and > >> hope you will include it. If not the case - close the ticket

> > > > To clarify in case it didn't come across clearly - I would have no > > qualms about the patch being rejected. However, regardless which way you > > decide to go, it would be nice to have a definitive answer. For instance > > if I knew you would be opposed to the idea, I would not have made the > > recommendation I made in [1]. Being informed helps ;)

> > Is this change necessary for Package::Stash to use M::I?

No, and I never claimed this. The change is necessary (imho) to make Module::Implementation more useful as a lowest level utility module. I never claimed anything more than this, hence explicitly noting that if you do not accept the patch as-is the world won't end. In fact my change to get rid of Try::Tiny had the extra benefit of attempting to expose the "should never happen" condition of an undefined $_ in a catch{}. Please consider incorporating at least some parts of _load_module_or_error() to replace the "under-the-rug sweep" added in bbaa0bac. Again - in *my* opinion, given the mission of M::I it must have an absolutely minimal dependency footprint, as long as it doesn't sacrifice its functionality and/or maintainability. My patchset does not sacrifice either, but at the end you are the primary maintainer so this decision lies with you. All I asked was a definitive yes/no, nothing more. Cheers

Fri Jan 04 04:58:38 2013 ribasushi [...] leporine.io - Correspondence added

On Tue Jan 01 21:58:52 2013, RIBASUSHI wrote: Show quoted text

> All I asked was a definitive yes/no, nothing more.

Any word on this? :)

Fri Jan 04 09:58:31 2013 autarch [...] urth.org - Correspondence added

Subject:	Re: [rt.cpan.org #82319] [PATCH] Minimize dependency footprint of Module::Implementation
Date:	Fri, 4 Jan 2013 08:58:21 -0600 (CST)
To:	Peter Rabbitson via RT <bug-Module-Implementation [...] rt.cpan.org>
From:	Dave Rolsky <autarch [...] urth.org>

On Fri, 4 Jan 2013, Peter Rabbitson via RT wrote: Show quoted text

> Queue: Module-Implementation > Ticket <URL: https://rt.cpan.org/Ticket/Display.html?id=82319 > > > On Tue Jan 01 21:58:52 2013, RIBASUSHI wrote:

>> All I asked was a definitive yes/no, nothing more.

> > Any word on this? :)

No on removing the deps, but I will look at the other changes you made. Thanks, -dave /*============================================================ http://VegGuide.org http://blog.urth.org Your guide to all that's veg House Absolute(ly Pointless) ============================================================*/

Fri Jan 04 10:55:38 2013 ribasushi [...] leporine.io - Correspondence added

On Fri Jan 04 09:58:31 2013, autarch@urth.org wrote: Show quoted text

> On Fri, 4 Jan 2013, Peter Rabbitson via RT wrote: >

> > Queue: Module-Implementation > > Ticket <URL: https://rt.cpan.org/Ticket/Display.html?id=82319 > > > > > On Tue Jan 01 21:58:52 2013, RIBASUSHI wrote:

> >> All I asked was a definitive yes/no, nothing more.

> > > > Any word on this? :)

> > No on removing the deps,

At least consider the changes to t/taint.t - this is a test you want to run under as many wacky environments as possible, to catch early freak-fails. Test::Taint is not necessary here as you are only using it to check if -T is in effect - the kill trick is equivalent.

Sun Jan 06 16:19:43 2013 DROLSKY [...] cpan.org - Correspondence added

I incorporated some of this, but I ended up keeping Test::Taint. I realized that my taint test didn't test much, since it didn't actually use a tainted $ENV{...} value.

Sun Jan 06 16:19:43 2013 DROLSKY [...] cpan.org - Status changed from 'open' to 'resolved'