Bug #82327 for Module-Implementation: [RFC] Augment/supplement the symbols argument with regex capabilities

Sat Dec 29 09:40:09 2012 ribasushi [...] leporine.io - Ticket created

Subject:

[RFC] Augment/supplement the symbols argument with regex capabilities

The use case is to replace this block [1] with the single call to Module::Implementation::build_loader_sub(). Specifically useful for Package::Stash since it invoking itself while loading makes it behave funny under older perls, and under the debugger. I am just handwaving the idea before writing code, to see if you agree with the idea in principle. I am thinhking that the regex interface is really only applicable to function names, hence symbols => [ 'a', 'b', qr/foo/ ] should DTRT. If you dislike the API duality - an alternative would be functions_matching => \@list_of_qrs, or something like that. Let me know what you think and I'll come up with the patches/tests/docs/etc. [1] https://github.com/carloslima/package-stash/blob/convert-to-module-implementation/lib/Package/Stash.pm#L20

Fri Jan 04 05:00:30 2013 ribasushi [...] leporine.io - Correspondence added

Any word on this? I got the go-ahead from doy to supply more patches to Package::Stash - implementing this would make for the cleanest approach of all. Cheers

Fri Jan 04 05:00:31 2013 ribasushi [...] leporine.io - Status changed from 'new' to 'open'

Fri Jan 04 09:59:29 2013 autarch [...] urth.org - Correspondence added

Subject:	Re: [rt.cpan.org #82327] [RFC] Augment/supplement the symbols argument with regex capabilities
Date:	Fri, 4 Jan 2013 08:59:19 -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=82327 > > > Any word on this? I got the go-ahead from doy to supply more patches to > Package::Stash - implementing this would make for the cleanest approach > of all.

Sure, I think allowing a single regex and using that to match sub names is the simplest way to go. -dave /*============================================================ http://VegGuide.org http://blog.urth.org Your guide to all that's veg House Absolute(ly Pointless) ============================================================*/

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

Subject:	Re: [rt.cpan.org #82327] [RFC] Augment/supplement the symbols argument with regex capabilities
Date:	Sat, 5 Jan 2013 02:19:46 +1100
To:	"autarch [...] urth.org via RT" <bug-Module-Implementation [...] rt.cpan.org>
From:	ribasushi [...] cpan.org

On Fri, Jan 04, 2013 at 09:59:29AM -0500, autarch@urth.org via RT wrote: Show quoted text

> <URL: https://rt.cpan.org/Ticket/Display.html?id=82327 > > > On Fri, 4 Jan 2013, Peter Rabbitson via RT wrote: >

> > Queue: Module-Implementation > > Ticket <URL: https://rt.cpan.org/Ticket/Display.html?id=82327 > > > > > Any word on this? I got the go-ahead from doy to supply more patches to > > Package::Stash - implementing this would make for the cleanest approach > > of all.

> > Sure, I think allowing a single regex and using that to match sub names is > the simplest way to go.

So then (to double check) something like: symbols => [qw(func1 func2 $globalvar %globalhash), qr/^any_method_like_/ ] I'll get you something towards the end of today if you find the syntax agreeable.

Fri Jan 04 10:32:07 2013 autarch [...] urth.org - Correspondence added

Subject:	Re: [rt.cpan.org #82327] [RFC] Augment/supplement the symbols argument with regex capabilities
Date:	Fri, 4 Jan 2013 09:31:58 -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=82327 > > > On Fri, Jan 04, 2013 at 09:59:29AM -0500, autarch@urth.org via RT wrote:

>> <URL: https://rt.cpan.org/Ticket/Display.html?id=82327 > >> >> On Fri, 4 Jan 2013, Peter Rabbitson via RT wrote: >>

>>> Queue: Module-Implementation >>> Ticket <URL: https://rt.cpan.org/Ticket/Display.html?id=82327 > >>> >>> Any word on this? I got the go-ahead from doy to supply more patches to >>> Package::Stash - implementing this would make for the cleanest approach >>> of all.

>> >> Sure, I think allowing a single regex and using that to match sub names is >> the simplest way to go.

> > So then (to double check) something like: > > symbols => [qw(func1 func2 $globalvar %globalhash), qr/^any_method_like_/ ] > > I'll get you something towards the end of today if you find the syntax > agreeable.

Yeah, that seems good. Actually, I guess we should allow >1 regex, since there's no reason to disallow it in the context of an array. -dave /*============================================================ http://VegGuide.org http://blog.urth.org Your guide to all that's veg House Absolute(ly Pointless) ============================================================*/

Mon Jul 08 01:19:34 2013 ribasushi [...] leporine.io - Correspondence added

On Fri Jan 04 10:32:07 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=82327 > > > > > On Fri, Jan 04, 2013 at 09:59:29AM -0500, autarch@urth.org via RT

> wrote:

> >> <URL: https://rt.cpan.org/Ticket/Display.html?id=82327 > > >> > >> On Fri, 4 Jan 2013, Peter Rabbitson via RT wrote: > >>

> >>> Queue: Module-Implementation > >>> Ticket <URL: https://rt.cpan.org/Ticket/Display.html?id=82327 > > >>> > >>> Any word on this? I got the go-ahead from doy to supply more

> patches to

> >>> Package::Stash - implementing this would make for the cleanest

> approach

> >>> of all.

> >> > >> Sure, I think allowing a single regex and using that to match sub

> names is

> >> the simplest way to go.

> > > > So then (to double check) something like: > > > > symbols => [qw(func1 func2 $globalvar %globalhash),

> qr/^any_method_like_/ ]

> > > > I'll get you something towards the end of today if you find the

> syntax

> > agreeable.

> > Yeah, that seems good. Actually, I guess we should allow >1 regex, > since > there's no reason to disallow it in the context of an array. >

https://github.com/ribasushi/module_implementation/compare/c2f67a8c...eab3c0b1 Patchset including this and other stuff. It is a lot of changes, but the commits are well isolated - review them one by one. Let me know if anything needs fixing etc.

Mon Jul 08 02:07:39 2013 ribasushi [...] leporine.io - Correspondence added

One more commit, cleans up an author test: https://github.com/ribasushi/module_implementation/commit/dfa78a05

Sun Jul 14 12:53:04 2013 DROLSKY [...] cpan.org - Correspondence added

On Mon Jul 08 02:07:39 2013, RIBASUSHI wrote: Show quoted text

> One more commit, cleans up an author test: > https://github.com/ribasushi/module_implementation/commit/dfa78a05

I took a look at this, and I like the general idea, but the implementation seems overly complicated. I don't like that regexes load copy _different_ symbols than strings. Why should only regexes look at multiple packages? What's the use case for using multiple implementations at all? If there is a use case, why should string matching apply here too? I'm also not sure it makes sense to limit regexes to just subroutines. It probably _does_ make sense to to require that a regex that wants to match some other type include the sigil. In other words, we could pass the symbols into the regex with their leading sigil _except_ for subs. That all said, I'd like to keep this as simple as possible. I'd be inclined to start with having regexes match subs only, and only look at one package. I made a copy of your fork in the branch "riba", which should now be visible on GitHub. I've made some style/naming tweaks in that branch, so it probably makes sense to do future work off of that. Thanks, -dave

Sun Jul 14 13:02:08 2013 DROLSKY [...] cpan.org - Correspondence added

On Mon Jul 08 02:07:39 2013, RIBASUSHI wrote: Show quoted text

> One more commit, cleans up an author test: > https://github.com/ribasushi/module_implementation/commit/dfa78a05

It also occurs to me that always loading all implementations is probably an anti-feature in some cases. It's possible that some implementations may use up a lot of memory. For example, maybe the module provides access to some sort of data set. One implementation uses an external file (Berkeley DB, whatever) and one provides it as an in-memory hash as a fallback. In that case you really don't want to load the in-memory hash version since maybe it uses 10MB of memory.

Sun Jul 14 15:53:01 2013 ribasushi [...] leporine.io - Correspondence added

Subject:	Re: [rt.cpan.org #82327] [RFC] Augment/supplement the symbols argument with regex capabilities
Date:	Sun, 14 Jul 2013 19:52:42 +0000
To:	Dave Rolsky via RT <bug-Module-Implementation [...] rt.cpan.org>
From:	Peter Rabbitson <ribasushi [...] cpan.org>

On Sun, Jul 14, 2013 at 12:53:04PM -0400, Dave Rolsky via RT wrote: Show quoted text

> <URL: https://rt.cpan.org/Ticket/Display.html?id=82327 > > > On Mon Jul 08 02:07:39 2013, RIBASUSHI wrote:

> > One more commit, cleans up an author test: > > https://github.com/ribasushi/module_implementation/commit/dfa78a05

> > I took a look at this, and I like the general idea, but the implementation seems overly complicated. > > I don't like that regexes load copy _different_ symbols than strings. Why should only regexes look at multiple packages?

I am confused by the above sentence. Please clarify so I can reply to the rest.

Sun Jul 14 16:09:51 2013 autarch [...] urth.org - Correspondence added

Subject:	Re: [rt.cpan.org #82327] [RFC] Augment/supplement the symbols argument with regex capabilities
Date:	Sun, 14 Jul 2013 15:09:36 -0500 (CDT)
To:	Peter Rabbitson via RT <bug-Module-Implementation [...] rt.cpan.org>
From:	Dave Rolsky <autarch [...] urth.org>

On Sun, 14 Jul 2013, Peter Rabbitson via RT wrote: Show quoted text

> Queue: Module-Implementation > Ticket <URL: https://rt.cpan.org/Ticket/Display.html?id=82327 > > > On Sun, Jul 14, 2013 at 12:53:04PM -0400, Dave Rolsky via RT wrote:

>> <URL: https://rt.cpan.org/Ticket/Display.html?id=82327 > >> >> On Mon Jul 08 02:07:39 2013, RIBASUSHI wrote:

>>> One more commit, cleans up an author test: >>> https://github.com/ribasushi/module_implementation/commit/dfa78a05

>> >> I took a look at this, and I like the general idea, but the implementation seems overly complicated. >> >> I don't like that regexes load copy _different_ symbols than strings. Why should only regexes look at multiple packages?

> > I am confused by the above sentence. Please clarify so I can reply to the rest.

Doh, I combined two things. One is that regexes only look at subs. The more I think about it the more I think that's a good idea. The second is that regexes look at multiple implementation packages. I don't understand the use case for this at all, especially since you explicitly check that implementations are consistent. -dave /*============================================================ http://VegGuide.org http://blog.urth.org Your guide to all that's veg House Absolute(ly Pointless) ============================================================*/

Mon Jul 15 04:15:56 2013 ribasushi [...] leporine.io - Correspondence added

Subject:	Re: [rt.cpan.org #82327] [RFC] Augment/supplement the symbols argument with regex capabilities
Date:	Mon, 15 Jul 2013 08:15:37 +0000
To:	"autarch [...] urth.org via RT" <bug-Module-Implementation [...] rt.cpan.org>
From:	Peter Rabbitson <ribasushi [...] cpan.org>

On Sun, Jul 14, 2013 at 04:09:52PM -0400, autarch@urth.org via RT wrote: Show quoted text

> <URL: https://rt.cpan.org/Ticket/Display.html?id=82327 > > > On Sun, 14 Jul 2013, Peter Rabbitson via RT wrote: >

> > Queue: Module-Implementation > > Ticket <URL: https://rt.cpan.org/Ticket/Display.html?id=82327 > > > > > On Sun, Jul 14, 2013 at 12:53:04PM -0400, Dave Rolsky via RT wrote:

> >> <URL: https://rt.cpan.org/Ticket/Display.html?id=82327 > > >> > >> On Mon Jul 08 02:07:39 2013, RIBASUSHI wrote:

> >>> One more commit, cleans up an author test: > >>> https://github.com/ribasushi/module_implementation/commit/dfa78a05

> >> > >> I took a look at this, and I like the general idea, but the implementation seems overly complicated. > >> > >> I don't like that regexes load copy _different_ symbols than strings. Why should only regexes look at multiple packages?

> > > > I am confused by the above sentence. Please clarify so I can reply to the rest.

> > Doh, I combined two things. One is that regexes only look at subs. The > more I think about it the more I think that's a good idea.

Right, as I do not see a way *how* to specify that a regex is supposed to match a specific symbol type. We could do (... { '*' => qr/.../ }... ) but this goes against simplicity. Show quoted text

> The second is that regexes look at multiple implementation packages. I > don't understand the use case for this at all, especially since you > explicitly check that implementations are consistent. >

This is *how* I check that implementations are consistent. If I do not look at all implementor-packages, I can not compile lists to compare. Now why the complexity of ensuring that implementation lists match: I wanted to prevent *extra* symbols being imported from one of the implementations without the user realizing it. Basically say you start with A (foo bar) and B (foo bar). You do an import based on qr/^[a-z]/i. Then you add 'baz_internal' to B - boom, you got an inconsistency you are not likely to even realize exists. Inspiration for this was Package::Stash itself - it blindly copies any sub: https://metacpan.org/source/DOY/Package-Stash-0.35/lib/Package/Stash.pm#L29 I am not hellbent on insisting this stays. But it seemed like the only right thing to do, *especially* given the open-endedness of the regex inclusion mechanism. As far as it being an anti-feature loading a lot of extra crap - if this is an issue, one can always request a specific implementation via the envvar, in which case the entire mechanism is circumvented. In any case - your call

Mon Jul 15 04:33:51 2013 ribasushi [...] leporine.io - Correspondence added

Subject:	Re: [rt.cpan.org #82327] [RFC] Augment/supplement the symbols argument with regex capabilities
Date:	Mon, 15 Jul 2013 08:33:36 +0000
To:	Dave Rolsky via RT <bug-Module-Implementation [...] rt.cpan.org>
From:	Peter Rabbitson <ribasushi [...] cpan.org>

On Sun, Jul 14, 2013 at 12:53:04PM -0400, Dave Rolsky via RT wrote: Show quoted text

> > What's the use case for using multiple implementations at all? If there is a use case, why should string matching apply here too? >

I now realized what confused you - I wrote the docs incorrectly. Of course the implementations from *only one* package are imported, no matter what. The scope that deals with symbol copy is here [1]. As you can see $from is set once and never varies. The rest (loading and scanning all implementations) is just a consistency check that happens once per compile time, as I explained earlier. [1] http://git.urth.org/cgit.cgi/Module-Implementation.git/tree/lib/Module/Implementation.pm?h=riba#n162

Mon Jul 22 11:53:54 2013 DROLSKY [...] cpan.org - Correspondence added

On Mon Jul 15 04:33:51 2013, RIBASUSHI wrote: Show quoted text

> On Sun, Jul 14, 2013 at 12:53:04PM -0400, Dave Rolsky via RT wrote:

> > > > What's the use case for using multiple implementations at all? If

> there is a use case, why should string matching apply here too?

> >

> > I now realized what confused you - I wrote the docs incorrectly. Of > course > the implementations from *only one* package are imported, no matter > what. > The scope that deals with symbol copy is here [1]. As you can see > $from > is set once and never varies. > > The rest (loading and scanning all implementations) is just a > consistency > check that happens once per compile time, as I explained earlier. > > [1] http://git.urth.org/cgit.cgi/Module- > Implementation.git/tree/lib/Module/Implementation.pm?h=riba#n162

I think it'd be best to make this consistency checking an opt-in thing. Module authors are probably in a better position to decide if it makes sense for their distros.

Mon Jul 22 16:37:22 2013 ribasushi [...] leporine.io - Correspondence added

Subject:	Re: [rt.cpan.org #82327] [RFC] Augment/supplement the symbols argument with regex capabilities
Date:	Mon, 22 Jul 2013 20:37:02 +0000
To:	Dave Rolsky via RT <bug-Module-Implementation [...] rt.cpan.org>
From:	Peter Rabbitson <ribasushi [...] cpan.org>

On Mon, Jul 22, 2013 at 11:53:54AM -0400, Dave Rolsky via RT wrote: Show quoted text

> <URL: https://rt.cpan.org/Ticket/Display.html?id=82327 > > > On Mon Jul 15 04:33:51 2013, RIBASUSHI wrote:

> > On Sun, Jul 14, 2013 at 12:53:04PM -0400, Dave Rolsky via RT wrote:

> > > > > > What's the use case for using multiple implementations at all? If

> > there is a use case, why should string matching apply here too?

> > >

> > > > I now realized what confused you - I wrote the docs incorrectly. Of > > course > > the implementations from *only one* package are imported, no matter > > what. > > The scope that deals with symbol copy is here [1]. As you can see > > $from > > is set once and never varies. > > > > The rest (loading and scanning all implementations) is just a > > consistency > > check that happens once per compile time, as I explained earlier. > > > > [1] http://git.urth.org/cgit.cgi/Module- > > Implementation.git/tree/lib/Module/Implementation.pm?h=riba#n162

> > I think it'd be best to make this consistency checking an opt-in thing. Module authors are probably in a better position to decide if it makes sense for their distros.

I think this is way too dangerous and messy with a regex based import. I was going to suggest a strictures-like check that will make the full-load run only when $0 =~ /t\// r somesuch. However if the best you can settle for is an opt-in, I think the entire "pick methods based on a regex" thing is a non-starter, and this patchset should go "resting" ;) Let me know what you think

Wed Jul 24 14:34:37 2013 DOY [...] cpan.org - Correspondence added

I agree with autarch - having your code die because of code that isn't even going to be used at all is not really that great of an idea.

Wed Jul 24 16:49:02 2013 ribasushi [...] leporine.io - Correspondence added

Alrighty. In this case I fully retract the feature request, since offering match-based imports from potentially differing implementations is an exceedingly bad idea. I'll rework the patchset later on to include just some of the cleanups. Please close this ticket and kill the branch in the repo. Cheers!

Sat Aug 03 12:49:04 2013 autarch [...] urth.org - Correspondence added

Subject:	Re: [rt.cpan.org #82327] [RFC] Augment/supplement the symbols argument with regex capabilities
Date:	Sat, 3 Aug 2013 11:48:48 -0500 (CDT)
To:	Peter Rabbitson via RT <bug-Module-Implementation [...] rt.cpan.org>
From:	Dave Rolsky <autarch [...] urth.org>

On Mon, 22 Jul 2013, Peter Rabbitson via RT wrote: Show quoted text

> I think this is way too dangerous and messy with a regex based import. I > was going to suggest a strictures-like check that will make the > full-load run only when $0 =~ /t\// r somesuch. > > However if the best you can settle for is an opt-in, I think the entire > "pick methods based on a regex" thing is a non-starter, and this > patchset should go "resting" ;) > > Let me know what you think

I think an opt-in is best, but I don't think that means the whole idea is not worth doing. But if you don't want it do it as an opt-in, that's ok. I may get around to adding this at some point myself. -dave /*============================================================ http://VegGuide.org http://blog.urth.org Your guide to all that's veg House Absolute(ly Pointless) ============================================================*/

Mon Aug 05 02:23:07 2013 ribasushi [...] leporine.io - Correspondence added

Subject:	Re: [rt.cpan.org #82327] [RFC] Augment/supplement the symbols argument with regex capabilities
Date:	Mon, 5 Aug 2013 06:22:53 +0000
To:	"autarch [...] urth.org via RT" <bug-Module-Implementation [...] rt.cpan.org>
From:	Peter Rabbitson <ribasushi [...] cpan.org>

On Sat, Aug 03, 2013 at 12:49:05PM -0400, autarch@urth.org via RT wrote: Show quoted text

> <URL: https://rt.cpan.org/Ticket/Display.html?id=82327 > > > On Mon, 22 Jul 2013, Peter Rabbitson via RT wrote: >

> > I think this is way too dangerous and messy with a regex based import. I > > was going to suggest a strictures-like check that will make the > > full-load run only when $0 =~ /t\// r somesuch. > > > > However if the best you can settle for is an opt-in, I think the entire > > "pick methods based on a regex" thing is a non-starter, and this > > patchset should go "resting" ;) > > > > Let me know what you think

> > I think an opt-in is best, but I don't think that means the whole idea is > not worth doing. But if you don't want it do it as an opt-in, that's ok. I > may get around to adding this at some point myself. >

Having a wildcard-based mechanism which does *not* check for consistency of the import lists is too dangerous to my taste. Even if the feature gets added I am not likely to use it, and wouldn't recommend someone does either. Hence the "kill the idea" idea ;)