Bug #63549 for Sphinx-Config: Round-trip safe

Thu Dec 02 15:30:13 2010 perl [...] pied.nu - Ticket created

Subject:

Round-trip safe

Config::Sphinx is not round-trip safe. That is if one reads in a config file, modifies it (or not) and writes it out any comments or special formating will be lost. Comments in a config file can be very important. My application needs Config::Sphinx to be round-trip safe. I have roughly an idea on how to do this simply. Would you be interested in a patch? Or shall I create a fork?

Thu Dec 02 16:02:40 2010 jon [...] jschutz.net - Correspondence added

Subject:	Re: [rt.cpan.org #63549] Round-trip safe
Date:	Fri, 03 Dec 2010 07:32:28 +1030
To:	bug-Sphinx-Config [...] rt.cpan.org
From:	Jon Schutz <jon [...] jschutz.net>

Hi Philip, Please, submit a patch. Regards, -- Jon Schutz http://notes.jschutz.net On 12/03/2010 07:00 AM, Philip Gwyn via RT wrote: Show quoted text

> Thu Dec 02 15:30:13 2010: Request 63549 was acted upon. > Transaction: Ticket created by GWYN > Queue: Sphinx-Config > Subject: Round-trip safe > Broken in: 0.01, 0.03, 0.04, 0.05 > Severity: Important > Owner: Nobody > Requestors: perl@pied.nu > Status: new > Ticket <URL: https://rt.cpan.org/Ticket/Display.html?id=63549 > > > > Config::Sphinx is not round-trip safe. That is if one reads in a config > file, modifies it (or not) and writes it out any comments or special > formating will be lost. > > Comments in a config file can be very important. My application needs > Config::Sphinx to be round-trip safe. > > I have roughly an idea on how to do this simply. Would you be > interested in a patch? Or shall I create a fork? >

Thu Dec 02 16:02:41 2010 The RT System itself - Status changed from 'new' to 'open'

Fri Dec 03 00:31:43 2010 perl [...] pied.nu - Correspondence added

Here is the first draft of the patch. I hope I've covered most edge cases, such as adding new variables, multi-variables. I've attempted to preserve the orignal whitespace intentions, where possible. It will even find commented out variables and put the new value after them.

Subject:

patch-Philip_Gwyn-Sphinx_Config-round_trip-01.patch

Message body is not shown because it is too large.

Fri Dec 03 00:53:16 2010 perl [...] pied.nu - Correspondence added

I should say that I've taken the liberty of adding a ->parse_string method. I also wrote POD for most of my changes. And a test unit for them. I also added a test unit for the simple document parsing.

Sun Dec 05 03:31:41 2010 jon.schutz [...] youramigo.com - Correspondence added

Subject:	Re: [rt.cpan.org #63549] Round-trip safe
Date:	Sun, 05 Dec 2010 19:01:20 +1030
To:	bug-Sphinx-Config [...] rt.cpan.org
From:	Jon Schutz <jon.schutz [...] youramigo.com>

Hi Philip, I'm seeing failing tests for config.t - not ok 7 - save # Failed test 'save' # at t/config.t line 26. # Structures begin differing at: # $got->[0]{_lines}[0] = '8' # $expected->[0]{_lines}[0] = '9' not ok 12 - save, no inheritance, child # Failed test 'save, no inheritance, child' # at t/config.t line 38. # got: 'mysql' # expected: 'pgsql' Also, I noted your comment "You may not set values in an C<indexer>, C<searchd> or C<search> section". Why would you impose such a limitation? Regards, -- Jon Schutz http://notes.jschutz.net On 12/03/2010 04:23 PM, Philip Gwyn via RT wrote: Show quoted text

> Queue: Sphinx-Config > Ticket <URL: https://rt.cpan.org/Ticket/Display.html?id=63549 > > > I should say that I've taken the liberty of adding a ->parse_string method. > > I also wrote POD for most of my changes. And a test unit for them. > > I also added a test unit for the simple document parsing. >

Mon Dec 06 10:32:16 2010 perl [...] pied.nu - Correspondence added

On Sun Dec 05 03:31:41 2010, jon.schutz@youramigo.com wrote: Show quoted text

> Hi Philip, > > I'm seeing failing tests for config.t -

I do not have config.t, it isn't part of the DIST. http://cpansearch.perl.org/src/JJSCHUTZ/Sphinx-Config-0.05/t/ Show quoted text

> Also, I noted your comment "You may not set values in an C<indexer>, > C<searchd> or C<search> section". Why would you impose such a >limitation?

I didn't; You did. As best I can make out from the code. Give me a snippet that will set variables in those sections and I'll make it work for round-trips.

Mon Dec 06 18:10:31 2010 jon.schutz [...] youramigo.com - Correspondence added

Subject:	Re: [rt.cpan.org #63549] Round-trip safe
Date:	Tue, 07 Dec 2010 09:40:10 +1030
To:	bug-Sphinx-Config [...] rt.cpan.org
From:	Jon Schutz <jon.schutz [...] youramigo.com>

Hi Philip, On 07/12/10 02:02, Philip Gwyn via RT wrote: Show quoted text

> Queue: Sphinx-Config > Ticket <URL: https://rt.cpan.org/Ticket/Display.html?id=63549 > > > On Sun Dec 05 03:31:41 2010, jon.schutz@youramigo.com wrote: >

>> Hi Philip, >> >> I'm seeing failing tests for config.t - >>

> I do not have config.t, it isn't part of the DIST. > http://cpansearch.perl.org/src/JJSCHUTZ/Sphinx-Config-0.05/t/ >

Indeed, so it isn't. I've fixed the MANIFEST for the next release and attached config.t here. Show quoted text

>

>> Also, I noted your comment "You may not set values in an C<indexer>, >> C<searchd> or C<search> section". Why would you impose such a >> limitation? >>

> I didn't; You did. As best I can make out from the code. > > Give me a snippet that will set variables in those sections and I'll > make it work for round-trips. >

There is a test case in the missing config.t: $c->set("indexer", undef, { mem_limit => "16M"}); is_deeply($c->get("indexer"), { mem_limit => "16M"}, "set complex var"); i.e. you can set $name to undef to access any of the unnamed sections like indexer, searchd. It looks to me like your code changes still handle this condition, so it's just the comment that needs to be fixed. Regards, -- Jon Schutz CTO, YourAmigo Ltd 53 Gilbert St Adelaide SA 5000 Ph: +61 8 82119211 Fax: +61 8 8211 6356 http://www.youramigo.com

Message body is not shown because sender requested not to inline it.

Tue Dec 07 00:01:03 2010 perl [...] pied.nu - Correspondence added

On Mon Dec 06 18:10:31 2010, jon.schutz@youramigo.com wrote: Show quoted text

> $c->set("indexer", undef, { mem_limit => "16M"}); > is_deeply($c->get("indexer"), { mem_limit => "16M"}, "set complex var");

A light just went on in my head. So *that*'s what that code was about. Do you want a full patch or a delta against my previous patch?

Tue Dec 07 00:09:46 2010 jon.schutz [...] youramigo.com - Correspondence added

Subject:	Re: [rt.cpan.org #63549] Round-trip safe
Date:	Tue, 07 Dec 2010 15:39:29 +1030
To:	bug-Sphinx-Config [...] rt.cpan.org
From:	Jon Schutz <jon.schutz [...] youramigo.com>

Show quoted text

> Do you want a full patch or a delta against my previous patch? >

Whichever is most convenient. -- Jon Schutz CTO, YourAmigo Ltd 53 Gilbert St Adelaide SA 5000 Ph: +61 8 82119211 Fax: +61 8 8211 6356 http://www.youramigo.com

Tue Dec 07 12:02:53 2010 perl [...] pied.nu - Correspondence added

Next question : There is currently no syntax to say that a new index or source section inherits from another section. If you don't want a round trip you would do: $S = $c->get( 'source' => 'foo' ); $c->set( source => bar => $S ); And get the same effect. What I want to do (I realised last night) is something like. $in = $c->get( source => 'foo' ); @out{ qw(some variables) } = @{$S}{ qw(some variables) }; $c->set( source => bar => \%out ); and have the other variables inherit from foo. How about: $c->set( source => bar => \%out, 'foo' ); Because 3rd param is a hashref, we know this a section-creation, not variable creation. It would produce source bar : foo { some = .... variables = ..... } What's more: $c->set( source => foo => undef(), 'zip' ); Changes source foo's parent section. And $c->get( source => bar => 'other' ); Must return source foo's 'other' variable. The other option is something like : $c->inherit( source => bar => 'foo' ); $c->set( source => bar => \%out ); This is more explicit, but less in the spirit of the DWIMitude of ->get() and ->set().

Tue Dec 07 14:59:47 2010 perl [...] pied.nu - Correspondence added

The current patch fixes config.t. config.t references sphinx.conf.dist, which I assume is the one that comes with sphinx, with a small change (src1stripped). On Tue Dec 07 12:02:53 2010, GWYN wrote: Show quoted text

> $c->set( source => foo => undef(), 'zip' );

I went with this syntax, exclusively. To create a new section with inheritance will require 2 calls to ->set()

Subject:

Philip_Gwyn-Sphinx_Config-round_trip-02.patch

Message body is not shown because it is too large.

Tue Dec 07 15:27:33 2010 perl [...] pied.nu - Correspondence added

On Tue Dec 07 14:59:47 2010, GWYN wrote: Show quoted text

> To create a new section with inheritance will require 2 calls to ->set()

OK. This patch allows one to do : $c->set( $type, $name, \%values, $base_name ) It completly superceeds the previous 2 patches.

Subject:

Philip_Gwyn-Sphinx_Config-round_trip-03.patch

Message body is not shown because it is too large.

Wed Dec 08 02:02:56 2010 jon.schutz [...] youramigo.com - Correspondence added

Subject:	Re: [rt.cpan.org #63549] Round-trip safe
Date:	Wed, 08 Dec 2010 17:32:22 +1030
To:	bug-Sphinx-Config [...] rt.cpan.org
From:	Jon Schutz <jon.schutz [...] youramigo.com>

Hi Philip, That all looks good. I've bundled it up into release 0.06 and uploaded to PAUSE. Thanks for your contribution. On 08/12/10 06:57, Philip Gwyn via RT wrote: Show quoted text

> Queue: Sphinx-Config > Ticket <URL: https://rt.cpan.org/Ticket/Display.html?id=63549 > > > On Tue Dec 07 14:59:47 2010, GWYN wrote: >

>> To create a new section with inheritance will require 2 calls to ->set() >>

> OK. This patch allows one to do : > > $c->set( $type, $name, \%values, $base_name ) > > It completly superceeds the previous 2 patches. > > >

-- Jon Schutz CTO, YourAmigo Ltd 53 Gilbert St Adelaide SA 5000 Ph: +61 8 82119211 Fax: +61 8 8211 6356 http://www.youramigo.com

Wed Dec 08 22:43:25 2010 perl [...] pied.nu - Correspondence added

You forgot all the config files that the unit tests use. -Philip

Wed Dec 08 23:05:26 2010 jon.schutz [...] youramigo.com - Correspondence added

Subject:	Re: [rt.cpan.org #63549] Round-trip safe
Date:	Thu, 09 Dec 2010 14:35:05 +1030
To:	bug-Sphinx-Config [...] rt.cpan.org
From:	Jon Schutz <jon.schutz [...] youramigo.com>

Fixed in 0.07 upload. On 09/12/10 14:13, Philip Gwyn via RT wrote: Show quoted text

> Queue: Sphinx-Config > Ticket <URL: https://rt.cpan.org/Ticket/Display.html?id=63549 > > > You forgot all the config files that the unit tests use. > > -Philip

-- Jon Schutz CTO, YourAmigo Ltd 53 Gilbert St Adelaide SA 5000 Ph: +61 8 82119211 Fax: +61 8 8211 6356 http://www.youramigo.com

Fri Dec 10 16:19:15 2010 perl [...] pied.nu - Correspondence added

Here is a small tweak against 0.07 that fixes 3 small things in my patch.

Subject:

Philip_Gwyn-Sphinx_Config-tweak-01.patch

diff -bruN Sphinx-Config-0.07-ORIG/lib/Sphinx/Config.pm Sphinx-Config-0.07-PG1/lib/Sphinx/Config.pm --- Sphinx-Config-0.07-ORIG/lib/Sphinx/Config.pm 2010-12-08 23:01:20.000000000 -0500 +++ Sphinx-Config-0.07-PG1/lib/Sphinx/Config.pm 2010-12-10 16:16:36.000000000 -0500 @@ -257,6 +257,7 @@ my $c; for (my $i = 0; $i <= $#$config; $i++) { $c = $config->[$i]; + next unless $c->{_name}; # ignore searchd, indexer sections if( $c->{_name} eq $name && $c->{_type} eq $type ) { return $c; } @@ -482,7 +483,7 @@ if( $value ) { # Change inheritance unless( $self->_change_inherit( $key, $value ) ) { - croak "Sphinx::Config: Unable to find $name $value for inheritance"; + croak "Sphinx::Config: Unable to find $type $value for inheritance"; } } } @@ -668,7 +669,7 @@ my $section = $self->{_keys}{ $key }; croak "Can't find section $key" unless $section; - return if $section->{_new}; + return 1 if $section->{_new}; my $file = $self->{_file}; my $pos = $section->{_lines};

Sat Dec 11 07:03:46 2010 jon.schutz [...] youramigo.com - Correspondence added

Subject:	Re: [rt.cpan.org #63549] Round-trip safe
Date:	Sat, 11 Dec 2010 22:33:25 +1030
To:	bug-Sphinx-Config [...] rt.cpan.org
From:	Jon Schutz <jon.schutz [...] youramigo.com>

Patch applied, uploaded as version 0.08. On 12/11/2010 07:49 AM, Philip Gwyn via RT wrote: Show quoted text

> Queue: Sphinx-Config > Ticket <URL: https://rt.cpan.org/Ticket/Display.html?id=63549 > > > Here is a small tweak against 0.07 that fixes 3 small things in my patch. > >

-- Jon Schutz CTO, YourAmigo Ltd 53 Gilbert St Adelaide SA 5000 Ph: +61 8 82119211 Fax: +61 8 8211 6356 http://www.youramigo.com

Wed Dec 15 18:43:41 2010 JJSCHUTZ [...] cpan.org - Status changed from 'open' to 'resolved'

Wed Dec 15 18:43:41 2010 JJSCHUTZ [...] cpan.org - Fixed in 0.08 added