Bug #48553 for MooseX-ConfigFromFile: Allow multiple configuration files

Sun Aug 09 05:55:52 2009 XSAWYERX [...] cpan.org - Ticket created

Subject:

Allow multiple configuration files

I already opened a ticket on MooseX::SimpleConfig, id 48552 [1], which has a patch to allow it to read multiple files. This should be backed by allowing MooseX::ConfigFromFile to receive an attribute that might be an array of a Path and not just a single Path. Included is a patch that does it. Thank you. [1] https://rt.cpan.org/Ticket/Display.html?id=48552

Subject:

mx_configfromfile-allow_multiple_files_attribute.patch

--- /usr/local/share/perl/5.8.8/MooseX/ConfigFromFile.pm 2008-01-23 07:53:48.000000000 +0200 +++ lib/MooseX/ConfigFromFile.pm 2009-07-19 20:22:18.000000000 +0300 @@ -9,7 +9,7 @@ has configfile => ( is => 'ro', - isa => File, + isa => 'Path::Class::File|ArrayRef[Path::Class::File]', coerce => 1, predicate => 'has_configfile', );

Mon Jan 25 09:18:57 2010 XSAWYERX [...] cpan.org - Correspondence added

On Sun Aug 09 05:55:52 2009, xsawyerx wrote: Show quoted text

> I already opened a ticket on MooseX::SimpleConfig, id 48552 [1], which > has a patch to allow it to read multiple files.

Now that ticket #48552 on MooseX::SimpleConfig was resolved, this would be an excellent time to include this patch. The patch might break coercion (I'm not sure), but that could be fixed. If that is the delay for including the patch, I can check and if needed add a manual coercion. Thank you. Show quoted text

> [1] https://rt.cpan.org/Ticket/Display.html?id=48552

Mon Jan 25 09:18:59 2010 XSAWYERX [...] cpan.org - Status changed from 'new' to 'open'

Thu Dec 16 14:13:08 2010 bobtfish [...] bobtfish.net - Correspondence added

Hiya. I'm now maintaining this module, is there any chance you could update your patch with some test cases to prove it does what you think it does?

Sun Apr 10 10:10:24 2011 XSAWYERX [...] cpan.org - Correspondence added

It took a loooong time for me to get back to this (I blame punk, hardcore and noise shows!) but here is a more full patch. It's still my original fix, but I've included a test by zby (props!) to it. It's done against a repo clone done a few minutes ago: diff --git a/lib/MooseX/ConfigFromFile.pm b/lib/MooseX/ConfigFromFile.pm index 2137247..acd2643 100644 --- a/lib/MooseX/ConfigFromFile.pm +++ b/lib/MooseX/ConfigFromFile.pm @@ -12,7 +12,7 @@ requires 'get_config_from_file'; has configfile => ( is => 'ro', - isa => File, + isa => 'Path::Class::File|ArrayRef[Path::Class::File]', coerce => 1, predicate => 'has_configfile', ); diff --git a/t/04multiple_files.t b/t/04multiple_files.t new file mode 100644 index 0000000..a4d55fa --- /dev/null +++ b/t/04multiple_files.t @@ -0,0 +1,27 @@ +#!/usr/bin/env perl +use strict; +use warnings; +use Test::More; +use Test::Fatal; + +{ + package Foo; + use Moose; + with qw(MooseX::ConfigFromFile); + + has bar => ( is => 'ro' ); + + has configfile => ( is => 'bare', default => sub { [ 'bar', 'baz' ] } ); + + sub get_config_from_file { + my $self = shift; + my $files = shift; + return { @{ $files->() } } + } +} + +is( exception { Foo->new_with_config() }, undef, 'Foo->new_with_config lives'); +my $foo = Foo->new_with_config(); +is( $foo->bar, 'baz', 'args to get_config_from_file' ); + +done_testing();

Sun Apr 10 10:30:55 2011 stevan.little [...] gmail.com - Correspondence added

Subject:	Re: [rt.cpan.org #48553] Allow multiple configuration files
Date:	Sun, 10 Apr 2011 10:30:41 -0400
To:	bug-MooseX-ConfigFromFile [...] rt.cpan.org
From:	Stevan Little <stevan.little [...] iinteractive.com>

Why not refactor some and make another role MooseX::ConfigFromFiles that will provide this functionality. I think it is very confusing to have a "configfile" attribute that might be a config file (as it says) and might be an array of config files. I think this new role could easily be included in this distro so that its not another CPAN module, but I think that while this is a good idea, the implementation adds confusion to the API. - Stevan On Apr 10, 2011, at 10:10 AM, Sawyer X via RT wrote: Show quoted text

> Queue: MooseX-ConfigFromFile > Ticket <URL: https://rt.cpan.org/Ticket/Display.html?id=48553 > > > It took a loooong time for me to get back to this (I blame punk, > hardcore and noise shows!) but here is a more full patch. It's still my > original fix, but I've included a test by zby (props!) to it. It's done > against a repo clone done a few minutes ago: > diff --git a/lib/MooseX/ConfigFromFile.pm b/lib/MooseX/ConfigFromFile.pm > index 2137247..acd2643 100644 > --- a/lib/MooseX/ConfigFromFile.pm > +++ b/lib/MooseX/ConfigFromFile.pm > @@ -12,7 +12,7 @@ requires 'get_config_from_file'; > > has configfile => ( > is => 'ro', > - isa => File, > + isa => 'Path::Class::File|ArrayRef[Path::Class::File]', > coerce => 1, > predicate => 'has_configfile', > ); > diff --git a/t/04multiple_files.t b/t/04multiple_files.t > new file mode 100644 > index 0000000..a4d55fa > --- /dev/null > +++ b/t/04multiple_files.t > @@ -0,0 +1,27 @@ > +#!/usr/bin/env perl > +use strict; > +use warnings; > +use Test::More; > +use Test::Fatal; > + > +{ > + package Foo; > + use Moose; > + with qw(MooseX::ConfigFromFile); > + > + has bar => ( is => 'ro' ); > + > + has configfile => ( is => 'bare', default => sub { [ 'bar', 'baz' ] > } ); > + > + sub get_config_from_file { > + my $self = shift; > + my $files = shift; > + return { @{ $files->() } } > + } > +} > + > +is( exception { Foo->new_with_config() }, undef, 'Foo->new_with_config > lives'); > +my $foo = Foo->new_with_config(); > +is( $foo->bar, 'baz', 'args to get_config_from_file' ); > + > +done_testing(); >

Sun Apr 10 10:44:00 2011 XSAWYERX [...] cpan.org - Correspondence added

On Sun Apr 10 10:30:55 2011, STEVAN wrote: Show quoted text

> Why not refactor some and make another role MooseX::ConfigFromFiles > that will provide this functionality. I think it is very confusing > to have a "configfile" attribute that might be a config file (as it > says) and might be an array of config files. > > I think this new role could easily be included in this distro so that > its not another CPAN module, but I think that while this is a good > idea, the implementation adds confusion to the API.

While I can see the "niceness" in the short run for a role that specifically does multiple files, in its core it will basically be a Copy-Paste of everything (including the tests) just to change one single line from allowing a single value to an arrayref of said value. On that account, MX::SimpleConfig should also be split into MX::SimpleConfig and MX::SimpleConfigs. Frankly, if it was a more serious change, I'd agree 100% but this is pretty small and seems more like a non-issue to me. Just my 0.02.

Sun Apr 10 20:13:56 2011 stevan.little [...] gmail.com - Correspondence added

Subject:	Re: [rt.cpan.org #48553] Allow multiple configuration files
Date:	Sun, 10 Apr 2011 20:13:40 -0400
To:	bug-MooseX-ConfigFromFile [...] rt.cpan.org
From:	Stevan Little <stevan.little [...] iinteractive.com>

On Apr 10, 2011, at 10:44 AM, Sawyer X via RT wrote: Show quoted text

> Queue: MooseX-ConfigFromFile > Ticket <URL: https://rt.cpan.org/Ticket/Display.html?id=48553 > > > On Sun Apr 10 10:30:55 2011, STEVAN wrote:

>> Why not refactor some and make another role MooseX::ConfigFromFiles >> that will provide this functionality. I think it is very confusing >> to have a "configfile" attribute that might be a config file (as it >> says) and might be an array of config files. >> >> I think this new role could easily be included in this distro so that >> its not another CPAN module, but I think that while this is a good >> idea, the implementation adds confusion to the API.

> > While I can see the "niceness" in the short run for a role that > specifically does multiple files, in its core it will basically be a > Copy-Paste of everything (including the tests) just to change one single > line from allowing a single value to an arrayref of said value. > > On that account, MX::SimpleConfig should also be split into > MX::SimpleConfig and MX::SimpleConfigs. > > Frankly, if it was a more serious change, I'd agree 100% but this is > pretty small and seems more like a non-issue to me. > > Just my 0.02.

I understand your point, but I think this is more an issue of the API being wrong and confusing. It shouldn't be too hard to refactor things into 3 roles, one with the core shared items, 1 to handle a single config file and 1 to handle multiple config files. - Stevan

Sat Feb 09 19:27:01 2013 ether [...] cpan.org - Severity Normal changed to Wishlist

Sat Feb 09 19:27:01 2013 ether [...] cpan.org - Broken in 0.01 deleted

Sat Feb 09 19:27:01 2013 ether [...] cpan.org - Broken in 0.02 deleted

Sat Nov 30 13:01:25 2013 ether [...] cpan.org - Status changed from 'open' to 'stalled'