Skip Menu |
 

This queue is for tickets about the Text-WikiFormat CPAN distribution.

Report information
The Basics
Id: 21269
Status: open
Worked: 1.5 hours (90 min)
Priority: 0/
Queue: Text-WikiFormat

People
Owner: chromatic [...] cpan.org
Requestors: derek [...] ximbiot.com
Cc:
AdminCc:

Bug Information
Severity: Critical
Broken in: 0.78
Fixed in: (no value)



Subject: Nesting forces ends to previous lists.
Download (untitled) / with headers
text/plain 356b
In a nest like the following (where # is ordered and * is unordered): # =1 # =2 ## =2.1 ##* =2.1.a ##* =2.1.b ## =2.2 # =3 The generated list looks like this: 1 =1 2 =2 1 =2.1 * =2.1.a * =2.1.b 1 =2.2 1 =3 The attached patch fixes this so that the generated HTML looks like this: 1 =1 2 =2 1 =2.1 * =2.1.a * =2.1.b 2 =2.2 3 =3
Subject: text-wikiformat-nests.diff
--- /usr/local/share/perl/5.8.7/Text/WikiFormat.pm 2006-07-27 10:06:46.000000000 -0400 +++ lib/Ximbiot/Text/WikiFormat.pm 2006-09-01 15:33:02.000000000 -0400 @@ -131,7 +131,6 @@ if exists $newtags->{blockorder} or exists $newtags->{blocks}; my @blocks = find_blocks( $text, \%tags, $opts ); - @blocks = merge_blocks( \@blocks ); @blocks = nest_blocks( \@blocks ); return process_blocks( \@blocks, \%tags, $opts ); } @@ -157,7 +156,7 @@ my ($text, $tags, $opts) = @_; my @blocks; - for my $line ( split(/\n/, $text) ) + for my $line ( split(/\r?\n/, $text) ) { my $block = start_block( $line, $tags, $opts ); push @blocks, $block if $block; @@ -182,14 +181,6 @@ } my $marker_removed = length ($line =~ s/$tags->{blocks}{$block}//); - if ($block eq 'code') - { - $level = 0; - $marker_removed = 1; - - # don't remove the indent, but do remove the code indent - ($line = $text) =~ s/$tags->{blocks}{code}//; - } next unless $marker_removed; --- /usr/local/share/perl/5.8.7/Text/WikiFormat/Blocks.pm 2006-07-27 10:06:46.000000000 -0400 +++ lib/Ximbiot/Text/WikiFormat/Blocks.pm 2006-09-01 15:29:23.000000000 -0400 @@ -120,6 +120,7 @@ { my ($self, $next_block) = @_; + return unless $next_block = $self->merge ($next_block); return $next_block unless $self->nests() and $next_block->nests(); return $next_block unless $self->level() < $next_block->level();
Download (untitled) / with headers
text/plain 369b
The attached patch includes a few other fixes so that the previous patch can pass the existing tests. Among other things, it fixes the merge_hash function so that hash elements are copied (recursively) rather than simply being moved into the new hash so that the %Text::WikiFormat::tags hash does not remember pollution from previous calls to Text::WikiFormat::format.

Message body is not shown because it is too large.

One more revision which doesn't mess so much with the import function.
Index: lib/Text/MediawikiFormat.pm =================================================================== RCS file: /cvsroot/Text-MediawikiFormat/lib/Text/MediawikiFormat.pm,v retrieving revision 1.2 diff -u -p -r1.2 MediawikiFormat.pm --- lib/Text/MediawikiFormat.pm 7 Sep 2006 20:05:33 -0000 1.2 +++ lib/Text/MediawikiFormat.pm 7 Sep 2006 20:52:54 -0000 @@ -8,7 +8,7 @@ use URI::Escape; use Text::MediawikiFormat::Blocks; use Scalar::Util qw( blessed reftype ); -use vars qw( $VERSION %tags %merge_matrix ); +use vars qw( $VERSION %tags %opts %merge_matrix ); $VERSION = '0.01'; %tags = ( indent => qr/^(?:\t+|\s{4,})/, @@ -48,11 +48,13 @@ $VERSION = '0.01'; schemas => [ qw( http https ftp mailto gopher ) ], ); +%opts = ( + implicit_links => 1, +); sub process_args { shift; # Class - return unless @_; return as => shift if @_ == 1; return as => 'wikiformat', @_; } @@ -61,7 +63,7 @@ sub process_args sub default_opts { return { - implicit_links => 1, + %opts, map { $_ => delete $_[1]->{ $_ } } qw( prefix extended implicit_links absolute_links ) }; @@ -176,39 +178,32 @@ sub merge_hash sub import { - my $class = shift; + return unless @_ > 1; + my $class = shift; my %args = $class->process_args( @_ ); - my $iopts = $class->default_opts( \%args ); my $name = delete $args{as}; - # Always define Text::MediawikiFormat::format if it hasn't been defined - # already, but only define a local function name if the importer asked - # for one. - my @names; - push @names, $class . "::format" unless $class->can ("format"); - if ($name) - { - my $caller = caller(); - push @names, $caller . "::" . $name; - } - - my $itags = merge_hash( \%args, \%tags); + my $caller = caller(); + my $iopts = $class->default_opts (\%args); + my $itags = merge_hash (\%args, \%tags); # Could verify ITAGS here via check_blocks, but what if a user # wants to add a block to block_order that they intend to override # the implementation of with every call to format()? - for my $fn (@names) + no strict 'refs'; + *{ $caller . "::" . $name } = sub { - no strict 'refs'; - *{ $fn } = sub - { - Text::MediawikiFormat::_format( $itags, $iopts, @_ ); - } + Text::MediawikiFormat::_format ($itags, $iopts, @_); } } +sub format +{ + _format (\%tags, \%opts, @_); +} + sub _format { my ($itags, $iopts, $text, $tags, $opts) = @_;
Download (untitled) / with headers
text/plain 376b
On Thu Sep 07 16:54:08 2006, DPRICE wrote: Show quoted text
> One more revision which doesn't mess so much with the import function.
These look like patches against Text::MediawikiFormat, which isn't mine. I do like the idea, however it seems better though to use Hash::Merge directly instead of duplicating its function. I've run into some trouble doing that, but I'll keep working on it.
Download (untitled) / with headers
text/plain 1.6k
On Mon Oct 09 22:25:11 2006, CHROMATIC wrote: Show quoted text
> These look like patches against Text::MediawikiFormat, which isn't mine.
Well, yes, T:MF is mine, but it was VERY closely based on Text::WikiFormat at the time I generated these patches. The patches should come close and I can work with you to resolve any differences. Show quoted text
> I do like the idea, however it seems better though to use Hash::Merge > directly instead of duplicating its function. I've run into some > trouble doing that, but I'll keep working on it.
Yes, I tried to use Hash::Merge initially, but I ran into a few difficulties as well. I forgot which problem made me decide to skip it and rewrite it, but I think it had to do with a bug in Hash::Merge. At the very least, H:M's default behavior is to copy blessed hashes without reblessing the result, which caused problems when I was trying to pass DB handles (actually, Wiki::Toolkit::Store::Database objects) to functions via the tags hash (so links may be colorized based on whether the node exists...). Another problem surfaced because H:M tries to merge arrays, when the documented behavior of things like your "schemas" and "unformatted_blocks" arrays is to override the previous settings. H:M is configurable enough that simply setting the default scalar and array behavior to copy should solve these two issues, if this isn't what triggered my bug. Then again, maybe I just decided Clone was too heavy a module to be justified. I don't recall anymore. Anyhow, the gist of this is, try out my H:M config array with H:M itself. It is probably still compatible with H:M. The patch I've already attached should also have tests that will demonstrate any incompatibilities with T:WF.
Subject: Re: [rt.cpan.org #21269] Nesting forces ends to previous lists.
Date: Mon, 9 Oct 2006 21:39:14 -0700
To: bug-Text-WikiFormat [...] rt.cpan.org
From: chromatic <chromatic [...] wgz.org>
Download (untitled) / with headers
text/plain 1.8k
On Monday 09 October 2006 19:46, Derek R. Price via RT wrote: Show quoted text
> > I do like the idea, however it seems better though to use Hash::Merge > > directly instead of duplicating its function. I've run into some > > trouble doing that, but I'll keep working on it.
> > Yes, I tried to use Hash::Merge initially, but I ran into a few > difficulties as well. I forgot which problem made me decide to skip it > and rewrite it, but I think it had to do with a bug in Hash::Merge. > > At the very least, H:M's default behavior is to copy blessed hashes > without reblessing the result, which caused problems when I was trying > to pass DB handles (actually, Wiki::Toolkit::Store::Database objects) to > functions via the tags hash (so links may be colorized based on whether > the node exists...).
Not an issue here, yet. Show quoted text
> Another problem surfaced because H:M tries to merge arrays, when the > documented behavior of things like your "schemas" and > "unformatted_blocks" arrays is to override the previous settings.
Yeah, that's the biggest problem now. Show quoted text
> H:M is configurable enough that simply setting the default scalar and > array behavior to copy should solve these two issues, if this isn't what > triggered my bug. Then again, maybe I just decided Clone was too heavy > a module to be justified. I don't recall anymore.
The problem is that Hash::Merge's code sucks; it uses UNIVERSAL::isa, which is always a mistake, ignores tied hashes, and makes you reconfigure everything if you just want to override one things. Show quoted text
> Anyhow, the gist of this is, try out my H:M config array with H:M > itself. It is probably still compatible with H:M. The patch I've > already attached should also have tests that will demonstrate any > incompatibilities with T:WF.
Do you have a test which demonstrates that the existing merge_hash() was buggy? -- c
Download (untitled) / with headers
text/plain 243b
Thanks, I applied most of the second patch. I found a way to work around the damage without pulling in so much of Hash::Merge's merge_hash(). I did leave the import() alone however; I didn't need to do anything else to get the tests to pass.
Download (untitled) / with headers
text/plain 309b
On Tue Oct 10 00:39:42 2006, chromatic@wgz.org wrote: Show quoted text
> Do you have a test which demonstrates that the existing merge_hash() > was buggy?
Yes. Sorry. I have three of them. I thought I had included them in the original patches. I'm attaching a diff now, in case you haven't duplicated all these tests yet.
Download merge-tests.diff
text/x-diff 2k
Index: t/merge-hash.t =================================================================== RCS file: /cvsroot/Text-MediawikiFormat/t/merge-hash.t,v retrieving revision 1.2 retrieving revision 1.4 diff -u -p -r1.2 -r1.4 --- t/merge-hash.t 5 Sep 2006 14:27:28 -0000 1.2 +++ t/merge-hash.t 8 Sep 2006 14:58:28 -0000 1.4 @@ -5,7 +5,7 @@ BEGIN { chdir 't' if -d 't' } use strict; use warnings; -use Test::More tests => 5; +use Test::More tests => 8; use_ok( 'Text::MediawikiFormat' ) or exit; my $full = { foo => { bar => 'baz' } }; @@ -15,18 +15,29 @@ my $full_flat = { a => 'b' }; my $empty_flat = {}; my $zero = { foo => 0, bar => { baz => 0 } }; -Text::MediawikiFormat::merge_hash( $full, $nonempty ); +$nonempty = Text::MediawikiFormat::merge_hash( $full, $nonempty ); is_deeply( $nonempty, { foo => { a => 'b', bar => 'baz' } }, "merge should work when all keys in from exist in to" ); +$full->{foo}->{bar} = 'boo'; +is_deeply( $nonempty, { foo => { a => 'b', bar => 'baz' } }, + "merge should copy subhashes" ); -Text::MediawikiFormat::merge_hash( $full_flat, $empty_flat ); +$empty_flat = Text::MediawikiFormat::merge_hash( $full_flat, $empty_flat ); is_deeply( $empty_flat, $full_flat, '... in flat case when keys exist in from but not in to' ); -Text::MediawikiFormat::merge_hash( $full, $empty ); +$empty = Text::MediawikiFormat::merge_hash( $full, $empty ); is_deeply( $empty, $full, '... in non-flat case when keys exist in but not in to' ); $empty = {}; -Text::MediawikiFormat::merge_hash( $zero, $empty ); +$empty = Text::MediawikiFormat::merge_hash( $zero, $empty ); is_deeply( $empty, $zero, '... and when value is zero but defined' ); + +my $regexer = { a => "regex" }; +my $arrayer = { a => ["X", "Y", "Z"] }; +my $merged; +$merged = Text::MediawikiFormat::merge_hash( $regexer, $arrayer ); +is_deeply( $merged, { a => "regex" }, "regexes should replace arrays" ); +$merged = Text::MediawikiFormat::merge_hash( $arrayer, $regexer ); +is_deeply( $merged, { a => ["X", "Y", "Z"] }, "...and vice versa" );


This service is sponsored and maintained by Best Practical Solutions and runs on Perl.org infrastructure.

Please report any issues with rt.cpan.org to rt-cpan-admin@bestpractical.com.