Bug #81793 for parent: allow version number in arg list

Sat Dec 08 03:08:55 2012 DAMI [...] cpan.org - Ticket created

Subject:

allow version number in arg list

To require a particular version of a parent module, we need to write use MyParent 1.234; use parent 'MyParent'; It would be convenient to support a syntax like use parent 'MyParent', -version => 1.234; or maybe just use parent 'MyParent', 1.234; # looks_like_number, must be a version num

Fri Feb 01 10:12:07 2013 rjbs [...] cpan.org - Correspondence added

On 2012-12-08 03:08:55, DAMI wrote: Show quoted text

> To require a particular version of a parent module, we need to write > > use MyParent 1.234; > use parent 'MyParent';

This came up on IRC yesterday and I also bemoaned its lack. I'd be happy to write a patch for this. I think this is exactly the kind of thing that parent.pm could get right that base.pm got wrong: provide a tiny but universally useful feature to avoid having to think very much. (As opposed to providing barely-ever features that lead to complications more often than they help.) -- rjbs

Fri Feb 01 10:12:08 2013 The RT System itself - Status changed from 'new' to 'open'

Fri Feb 01 10:39:26 2013 rjbs [...] cpan.org - Correspondence added

I've attached two small patches. Together they allow: use parent Something => { version => 1.2 }; and also use parent Something => { no_require => 1 }; And also fix a bug whereby no_require exempted one from the self-inherit check. If these look okay, I will see about also writing tests. ;) -- rjbs

Subject:

0001-add-assert-version-and-fix-bugs.patch

From 4b1a8498b5d54ad96e469e0ff5d15b7ecd1e9107 Mon Sep 17 00:00:00 2001 From: Ricardo Signes <rjbs@cpan.org> Date: Fri, 1 Feb 2013 10:26:05 -0500 Subject: [PATCH 1/3] add "assert version" and fix bugs... 1. do not allow '-norequire' to bypass self-inherit check 2. allow "use parent foo => { ':version' => 1.23 }" --- lib/parent.pm | 27 ++++++++++++++++++--------- 1 file changed, 18 insertions(+), 9 deletions(-) diff --git a/lib/parent.pm b/lib/parent.pm index 8f72db8..905b4c9 100755 --- a/lib/parent.pm +++ b/lib/parent.pm @@ -7,23 +7,32 @@ sub import { my $class = shift; my $inheritor = caller(0); + my @to_push; + my $no_require; if ( @_ and $_[0] eq '-norequire' ) { shift @_; - } else { - for ( my @filename = @_ ) { - if ( $_ eq $inheritor ) { - warn "Class '$inheritor' tried to inherit from itself\n"; - }; - - s{::|'}{/}g; - require "$_.pm"; # dies if the file is not found + $no_require = 1; + } + + my @todo = @_; + while (my $name = shift @todo) { + if ( $name eq $inheritor ) { + warn "Class '$inheritor' tried to inherit from itself\n"; + }; + + push @to_push, $name; + $name =~ s{::|'}{/}g; + require "$name.pm" unless $no_require; # dies if the file is not found + if (@todo and ref $todo[0]) { + $name->VERSION($todo[0]{':version'}); + shift @todo; } } { no strict 'refs'; - push @{"$inheritor\::ISA"}, @_; + push @{"$inheritor\::ISA"}, @to_push; }; }; -- 1.8.1

Subject:

0002-allow-no_require-per-parent-class.patch

From fef3233b30e2245d12e17a36bca5db8e8ea6aa52 Mon Sep 17 00:00:00 2001 From: Ricardo Signes <rjbs@cpan.org> Date: Fri, 1 Feb 2013 10:30:05 -0500 Subject: [PATCH 2/3] allow no_require per parent class As long as we have "parent class options," allow no_require per class. This is almost certainly a better plan, anyway, for any case in which we're doing MI. --- lib/parent.pm | 14 ++++++++++---- 1 file changed, 10 insertions(+), 4 deletions(-) diff --git a/lib/parent.pm b/lib/parent.pm index 905b4c9..6f3fe07 100755 --- a/lib/parent.pm +++ b/lib/parent.pm @@ -21,13 +21,19 @@ sub import { warn "Class '$inheritor' tried to inherit from itself\n"; }; - push @to_push, $name; - $name =~ s{::|'}{/}g; - require "$name.pm" unless $no_require; # dies if the file is not found + my %arg = (no_require => $no_require); if (@todo and ref $todo[0]) { - $name->VERSION($todo[0]{':version'}); + %arg = (%arg, %{ $todo[0] }); shift @todo; } + + push @to_push, $name; + $name =~ s{::|'}{/}g; + + # dies if the file is not found + require "$name.pm" unless $arg{no_require}; + + $name->VERSION($arg{version}) if defined $arg{version}; } { -- 1.8.1

Fri Feb 01 10:41:08 2013 corion [...] cpan.org - Correspondence added

Subject:	Re: [rt.cpan.org #81793] allow version number in arg list
Date:	Fri, 01 Feb 2013 16:40:48 +0100
To:	bug-parent [...] rt.cpan.org
From:	Max Maischein <corion [...] cpan.org>

Hello Damien and Ricardo, Show quoted text

>> use MyParent 1.234; >> use parent 'MyParent';

> > This came up on IRC yesterday and I also bemoaned its lack. I'd be happy to write a patch for > this. I think this is exactly the kind of thing that parent.pm could get right that base.pm got > wrong: provide a tiny but universally useful feature to avoid having to think very much. (As > opposed to providing barely-ever features that lead to complications more often than they help.) >

I already wrote to David Golden in a related mail that I'm opposed to this change. I don't see any big gain over the two-line approach of use 'Some::Parent' 1.23; use parent 'Some::Parent'; or use parent 'Some::Parent'; Some::Parent->VERSION(1.23); ... and the added complexity and feature creep is what made base.pm such a mess. At least in my perception, the group of you three is the only instance wanting to enforce a version number. I admit that I'm biased as I don't have such a use-case myself. I especially want to avoid one of the really ugly problems of base.pm, which forced a version number for modules that didn't provide a version number themselves ("-1, set by base.pm"). David proposed an API of use parent [ 'Some::Parent' => 1.234 ], [ 'Another::Parent' => 5.678 ]; but I don't like this idea either. Back when parent.pm was born, there was a quite similar discussion except for the "-norequire" switch. Originally, there was the idea of allowing (I think) hashrefs to signify parent classes that should not get loaded from disk, but we arrived at the design decision of either having '-norequire' as the first argument and then not trying to load any of the classes, or loading all of the classes. One way forward I could see would be another module, parent/versions.pm, which is basically a small loop around parent.pm: package parent::versions; require parent; sub import { my $class= shift; my $inheritor= caller(0); local *CORE::GLOBAL::caller = sub { $inheritor }; while(( my $module, $version ) = splice @_, 0, 2) { parent::->import($module); $module->VERSION($version); }; }; 1 To get around the nasty *CORE::GLOBAL::caller thing, which depends on the load order of parent.pm, I can think of moving the main code of parent.pm to a subroutine that takes the target namespace as an argument instead of inferring it from caller(0). -max

Fri Feb 01 10:59:16 2013 corion [...] cpan.org - Correspondence added

Subject:	Re: [rt.cpan.org #81793] allow version number in arg list
Date:	Fri, 01 Feb 2013 16:58:54 +0100
To:	bug-parent [...] rt.cpan.org
From:	Max Maischein <corion [...] cpan.org>

Hello Ricardo, Show quoted text

> I've attached two small patches. Together they allow: > > use parent Something => { version => 1.2 }; > > and also > > use parent Something => { no_require => 1 }; > > And also fix a bug whereby no_require exempted one from the self-inherit check. > > If these look okay, I will see about also writing tests. ;)

I haven't looked at them yet. There was discussion in 2007 ([1] and [2]) which led to the current behaviour+API. I don't see why the current behaviour should be changed, especially in the light of adding the feature outside of parent.pm, maybe with a little help from parent.pm by making the API less implicit. -max [1] http://www.nntp.perl.org/group/perl.perl5.porters/2007/08/msg127450.html [2] http://www.nntp.perl.org/group/perl.perl5.porters/2007/08/msg127774.html

Fri Feb 01 10:59:41 2013 dagolden [...] cpan.org - Correspondence added

I created a trivial fork called 'inherit' use inherit 'Foo' => 1.23, 'Bar'; The implementation passes all of parent's tests, plus additional version check testing. https://github.com/dagolden/inherit/blob/master/lib/inherit.pm I'm happy to provide it in patch format for parent (or patch it directly in blead). Or, if you really feel it's too much feature creep, I'll just release it to CPAN. FWIW, the argument "why not type it out longhand" applies to base/parent in the first place, so I don't find it compelling.

Fri Feb 01 11:09:05 2013 rjbs [...] cpan.org - Correspondence added

On 2013-02-01 10:41:08, CORION wrote: Show quoted text

> I already wrote to David Golden in a related mail that I'm opposed to > this change. I don't see any big gain over the two-line approach of > > use 'Some::Parent' 1.23; > use parent 'Some::Parent';

Isn't this like "I don't see any big gain of use parent over; use SomeParent 1.23; BEGIN { push our @ISA, 'SomeParent' }" ? Show quoted text

> I especially want to avoid one of the really ugly problems of base.pm, > which forced a version number for modules that didn't provide a > version > number themselves ("-1, set by base.pm").

Nobody is suggesting restoring such nonsense! Certainly not me, and if David Golden is going to argue for anything, I'm sure it wouldn't be this. ;) Show quoted text

> local *CORE::GLOBAL::caller = sub { $inheritor }; > ...

Show quoted text

> To get around the nasty *CORE::GLOBAL::caller thing, which depends on > the load order of parent.pm, I can think of moving the main code of > parent.pm to a subroutine that takes the target namespace as an > argument instead of inferring it from caller(0).

This seems like contortions in service of conceptual purity that doesn't have much practicla value. -- rjbs

Fri Feb 01 11:23:11 2013 corion [...] cpan.org - Correspondence added

Subject:	Re: [rt.cpan.org #81793] allow version number in arg list
Date:	Fri, 01 Feb 2013 17:22:54 +0100
To:	bug-parent [...] rt.cpan.org
From:	Max Maischein <corion [...] cpan.org>

Hello David, Show quoted text

> I created a trivial fork called 'inherit' > > use inherit 'Foo' => 1.23, 'Bar'; > > The implementation passes all of parent's tests, plus additional version > check testing. > > https://github.com/dagolden/inherit/blob/master/lib/inherit.pm > > I'm happy to provide it in patch format for parent (or patch it directly > in blead). > > Or, if you really feel it's too much feature creep, I'll just release it > to CPAN. > > FWIW, the argument "why not type it out longhand" applies to base/parent > in the first place, so I don't find it compelling. >

Well, the longhand for base / parent is about four lines: BEGIN { require Some::Parent; push @ISA, 'Some::Parent'; }; ... while I consider version checking a rare requirement, which can live better with the two-line longhand. Determining the version number the way you do is nice, but I would prefer to disallow v-strings resp. quoted-things-that-look-like-v-strings altogether. This also would remove the problem of not allowing use inherit 'Foo' => v2; while allowing use inherit 'Foo' => v2.0; Also, your approach fails with: use inherit 'Foo' => v65.0; v-strings should just die. I'm not sure whether the API of parent.pm should allow for the fancy version syntax by sacrificing package names that look like version numbers. That said, I don't think numeric package names make sense. While I'm still not convinced of the utility of the change, I consider the API somewhat nicer than the other alternatives that use hashrefs or arrayrefs. -max

Fri Feb 01 11:33:02 2013 corion [...] cpan.org - Correspondence added

Subject:	Re: [rt.cpan.org #81793] allow version number in arg list
Date:	Fri, 01 Feb 2013 17:32:41 +0100
To:	bug-parent [...] rt.cpan.org
From:	Max Maischein <corion [...] cpan.org>

Hello Ricardo, Show quoted text

> On 2013-02-01 10:41:08, CORION wrote:

>> I already wrote to David Golden in a related mail that I'm opposed to >> this change. I don't see any big gain over the two-line approach of >> >> use 'Some::Parent' 1.23; >> use parent 'Some::Parent';

> > Isn't this like "I don't see any big gain of use parent over; use SomeParent 1.23; BEGIN { push > our @ISA, 'SomeParent' }" ?

Yep, it's just shorter to type, and the version number is a use case that I find rare. Show quoted text

>> I especially want to avoid one of the really ugly problems of base.pm, >> which forced a version number for modules that didn't provide a >> version >> number themselves ("-1, set by base.pm").

> > Nobody is suggesting restoring such nonsense! Certainly not me, and if David Golden is > going to argue for anything, I'm sure it wouldn't be this. ;) >

>> local *CORE::GLOBAL::caller = sub { $inheritor }; >> ...

>

>> To get around the nasty *CORE::GLOBAL::caller thing, which depends on >> the load order of parent.pm, I can think of moving the main code of >> parent.pm to a subroutine that takes the target namespace as an >> argument instead of inferring it from caller(0).

> > This seems like contortions in service of conceptual purity that doesn't have much practicla > value.

I see an easy way to add this feature outside of parent.pm by doing a small, non-invasive change to parent.pm itself, which allows it to be used from other modules in an easy way. Which is why I proposed this change, so the intended functionality can be added in a separate module without changing the existing API and parameter convention of parent.pm. Especially in the light of three different approaches, DWIM at the cost of disallowing numeric package names, and explicit, using hashrefs or arrayrefs, I think that the best API for passing a version requirement isn't obvious, and trying out the alternatives is better done outside parent.pm. -max

Fri Feb 01 11:58:41 2013 dagolden [...] cpan.org - Correspondence added

I agree v-strings suck, but I'd like to preserve anything that "use MODULE VERSION" can take. Taking them as strings is actually safer/better, but requires Perl v5.10 or version.pm. Either approach is fine with me. I'd probably go the version.pm route for a CPAN release. Admittedly, the "v65.0" case is slightly tricky. There is a way to introspect a scalar to see if it's v-string, but takes some B hackery.

Fri Feb 01 13:04:18 2013 ether [...] cpan.org - Requestor ETHER added

Fri Feb 01 19:57:59 2013 dagolden [...] cpan.org - Requestor DAGOLDEN added

Fri Feb 01 21:25:42 2013 dagolden [...] cpan.org - Correspondence added

I've updated inherit.pm on github to address concerns raised. Specifically: (1) potential version constraints are tested explicitly against a full module-name regex and only considered a version if that fails (2) dotted-decimals are documented as needing to be in strings, not raw v-strings (this requires adding version.pm) (3) we can still handle raw v-strings that don't map to alphanumerics (thanks to #1), but this is documented as strongly discouraged (4) uses (updated) Module::Load to ensure things that look like modules are only loaded from @INC Please consider whether this is sufficient to be ported back to parent. If not (or if I haven't heard back by the middle of next week or so), I'll go ahead and release inherit.pm to CPAN instead. I'd like it to go in, but I respect an alternative point of view with different priorities. :-) Regards, David

Sat Feb 02 07:51:02 2013 laurent.dami [...] free.fr - Correspondence added

Subject:	Re: [rt.cpan.org #81793] allow version number in arg list
Date:	Sat, 02 Feb 2013 13:50:54 +0100
To:	bug-parent [...] rt.cpan.org
From:	laurent dami <laurent.dami [...] free.fr>

Le 02.02.2013 03:25, David Golden via RT a écrit : Show quoted text

> <URL: https://rt.cpan.org/Ticket/Display.html?id=81793 > > > I've updated inherit.pm on github to address concerns raised. > > [...] > > Please consider whether this is sufficient to be ported back to parent. >

Thanks for this work; to me this looks like a perfect answer to my original ticket. I hope this will reach an agreement and get implemented within parent.pm, instead of a new module inherit.pm; otherwise I'll have to tell my team that after having changed all our code from 'base.pm' to 'parent.pm', we now change again .... for sure they will complain ... :-( !

Sat Feb 02 09:04:44 2013 dagolden [...] cpan.org - Correspondence added

Subject:	Re: [rt.cpan.org #81793] allow version number in arg list
Date:	Sat, 2 Feb 2013 09:04:04 -0500
To:	bug-parent [...] rt.cpan.org
From:	David Golden <dagolden [...] cpan.org>

On Sat, Feb 2, 2013 at 7:51 AM, laurent.dami@free.fr via RT <bug-parent@rt.cpan.org> wrote: Show quoted text

> I hope this will reach an agreement and get implemented within > parent.pm, instead of a new module inherit.pm; otherwise I'll have to > tell my team that after having changed all our code from 'base.pm' to > 'parent.pm', we now change again .... for sure they will complain ... :-( !

The tricky bit is that you'll have to specify a newer version of parent in your prereqs. I hope if this change goes through, we could bump the major version to "1", so people can easily remember what version to specify instead of having to remember '0.2XX' or whatever it turns out to be. I routinely get annoyed trying to remember which version of File::Temp implemented 'newdir' or which Test::More added subtests. -- David Golden <dagolden@cpan.org> Take back your inbox! → http://www.bunchmail.com/ Twitter/IRC: @xdg

Mon Mar 09 14:57:35 2015 corion [...] cpan.org - Correspondence added

The functionality discussed in this ticket lives now in superclass.pm on CPAN.

Mon Mar 09 14:57:37 2015 corion [...] cpan.org - Status changed from 'open' to 'rejected'

Mon Mar 09 14:57:37 2015 corion [...] cpan.org - Fixed in 0.229 added