Skip Menu |
 

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

Report information
The Basics
Id: 57905
Status: open
Priority: 0/
Queue: Text-Autoformat

People
Owner: Nobody in particular
Requestors: wright [...] pair.com
Cc:
AdminCc:

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



CC: bug-Text-Autoformat [...] rt.cpan.org
Subject: Text-Autoformat: Proposed new feature to 'ignore' and 'mail' params.
Date: Thu, 27 May 2010 10:38:48 -0400 (EDT)
To: damian [...] conway.org
From: Dan Wright <wright [...] pair.com>
Download (untitled) / with headers
text/plain 1.6k
Hi, I've started using Text::Autoformat in one of my projects at work, and I'm enjoying the module. I've noticed a few quirks regarding the 'mail' & 'ignore' parameters and I'd like to suggest an improvement: First, even though the docs don't say so, it is currently impossible to use both the 'mail' and 'ignore' parameters together. Any use of 'ignore' currently overrides 'mail'. Additionally, it is a little frustrating that currently you can only ignore one thing without constructing your own code-ref. And, there seems to be no way to ignore 'indented' in addition to something else. Finally, a rather minor point: The docs mention that 'mail' implies all, but do not mention that 'ignore' also implies all. My proposal is that autoformat be allowed to ignore more than one thing at a time. I suggest that the ignore parameter accept an array-ref of ignores, and that the 'mail' and 'ignore' parameters should not be mutually exclusive. It would allow for the following syntax: $tidy = autoformat $messy, { ignore => [ 'indented', qr/one/, qr/two/ ], mail => 1 }; The only case where this change would not be backwards compatible would be instances where users are currently using both 'mail' and 'ignore' right now. But, I think if anybody is doing that, they probably are unaware that currently their mail headers are all jumbled up. So, this fix probably puts them closer to what they were expecting. I've attached a proposed diff and regression tests. Thanks and best regards, -Dan Daniel J. Wright wright@pair.com Lead Software Developer, pairNIC https://www.pairnic.com pair Networks, Inc. http://www.pair.com

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

Download 01.ignore.t
text/x-perl 2.1k

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

Download (untitled) / with headers
text/plain 1.2k
On Thu May 27 10:38:58 2010, wright@pair.com wrote: Show quoted text
> I've attached a proposed diff and regression tests.
Hello, Unfortunately, the patch has a bug. The if block in _build_ignore lacks a closing "else." If a coderef is passed to _build_ignore as the $ignore_arg, it is never set into $ignore. This causes it to croak with "Expected suboutine reference as value for -ignore option" ironically only in the case when you pass a code reference. My proposed patch: --- /usr/local/lib/perl5/site_perl/5.8.3/Text/Autoformat.pm.orig Wed Jun 2 10:35:50 2010 +++ /usr/local/lib/perl5/site_perl/5.8.3/Text/Autoformat.pm Wed Jun 2 10:38:21 2010 @@ -484,6 +484,8 @@ } elsif ($ignore_arg =~ /^indent/i) { $ignore = sub { ignore_headers(@_) || /$ignore_indent/ }; + } else { + $ignore = $ignore_arg; } croak "Expected suboutine reference as value for -ignore option" if ref $ignore ne 'CODE'; An alternative would be to use "my $ignore = $ignore_arg" and maintain the missing else block. Thanks! Alan Ferrency pair Networks alan@pair.com Show quoted text
> > Thanks and best regards, > -Dan > > > Daniel J. Wright wright@pair.com > Lead Software Developer, pairNIC https://www.pairnic.com > pair Networks, Inc. http://www.pair.com


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.