[perl #24821] enhancement patch for B::Concise
Jim Cromie
# Please include the string: [perl #24821]
# in the subject line of all future correspondence about this issue.
# <URL: http://rt.perl.org/rt3/Ticket/Display.html?id=24821 >
in case your mail browser doesnt inline the 1st attachment, heres the
highlites:
The patch adds 3 things:
1. B::Concise::walk_output($fh), with which you can send Concise output
to a file, string (ala: open(my $fh,'>',\$string)), etc.
2. B::Concise::add_style(), with which you can add a named style to
those already available. This is mostly convenience, but it lets
you register new styles, then use them like theyre built-in ( including
naming them on the command-line, or as args to B::Concise::compile() )
3. Tests for above, inc test following pod recipe for using B::Concise
outside O framework.
Tested against bleadperl. all OK.
h...@crypt.org
:+sub walk_output {
:+ $walkHandle = shift;
:+ my $iotype = ref $walkHandle;
:+ die "expecting GLOB, FILEHANDLE or IO::* argument"
:+ unless $iotype eq 'GLOB' or $iotype =~ /^IO::/;
:+}
This sets my alarm bells ringing: if I have a 'Hugo::File' that
inherits from an appropriate IO::* module, this will fail to accept it.
I don't think we really have good mechanisms for checking this sort of
thing cleanly - perl6 is shaping up to be much better in that respect -
but one approach would be to check that $walkHandle->can() each of the
methods that you need it to support.
But the patch as a whole looks interesting, thank you. :)
Hugo
Yitzchak Scott-Thoennes
> --- bleadperl/ext/B/t/concise.t Tue Feb 4 14:17:24 2003
> +++ bleadconcise/ext/B/t/concise.t Mon Jan 5 19:20:58 2004
> +# set up 'filehandles' for testing walk_output()
> +my ($s1,$s2,$s3);
> +open(my $f1, '>', \$s1);
> +open(my $f2, '>', \$s2);
> +open(my $f3, '>', \$s3);
Will those opens work without perlio? Do we care?
> +my $ref = <<'EORef';
> +7 <1> leavesub[1 ref] K/REFC,1 ->(end)
> +- <@> lineseq KP ->7
> +1 <;> nextstate(main -\d+ concise.t:\d+) v ->2
> +6 <2> sassign sKS/2 ->7
> +4 <2> add[t1] sK/2 ->5
> +- <1> ex-rv2sv sK/1 ->3
> +2 <$> gvsv(*b) s ->3
> +3 <$> const(IV 42) s ->4
> +- <1> ex-rv2sv sKRM*/1 ->6
> +5 <$> gvsv(*a) s ->6
> +EORef
> +
> +# this test is probably somewhat brittle, despite the qr//
> +like ($s2, /\Q$ref/, "matches against hardcoded reference output");
I think that like() is getting passed "" as the pattern to check, the *result* of a
match against $_; probably not what you intended. Even changed to use
qr//, I don't think you want the \Q, that will make your \d+ literal.
You need to go through and manually backslash all the $*()[].@ chars.
> +# try more robust test, with brittle lib inclusion
> +SKIP: {
> + use lib '/usr/local/lib/perl5/site_perl/5.8.2/i686-linux-thread-multi';
> +
> + eval "use String::Approx 'amatch'";
> + skip("no String::Approx, $@", 1) if $@;
> +
> + my @matches = amatch($s1, $s2);
> +
> + print "@matches\n";
> + ok(1, "I dont have it!");
> +}
Not sure exactly what that's for, but you may want instead:
use Config;
use lib $Config::Config{sitelib};
lib.pm is supposed to automatically also add the archname subdirectory
if it has stuff in it.
Paul Johnson
Jim Cromie said:
> 3. Tests for above, inc test following pod recipe for using B::Concise
> outside O framework.
Thanks very much. Presumably I hadn't added any tests because you hadn't
added the framework for them yet. Yes, that must be it.
--
Paul Johnson - pa...@pjcj.net
http://www.pjcj.net
Jim Cromie
>Jim Cromie said:
>
>
>
>>3. Tests for above, inc test following pod recipe for using B::Concise
>>outside O framework.
>>
>>
>
>Thanks very much. Presumably I hadn't added any tests because you hadn't
>added the framework for them yet. Yes, that must be it.
>
>
>
huh ?
Ok, youre Devel::Cover Dude.
I see the following in your pod, but I dont see how it relates.
Could you elaborate ?
If the module does not use the t/*.t framework:
=head1 ACKNOWLEDGEMENTS
Some code and ideas cribbed from:
Devel::OpProf
B::Concise
Jim Cromie
>On Tue, Jan 06, 2004 at 02:49:59AM -0000, Jim Cromie <perlbug-...@perl.org> wrote:
>
>
>>--- bleadperl/ext/B/t/concise.t Tue Feb 4 14:17:24 2003
>>+++ bleadconcise/ext/B/t/concise.t Mon Jan 5 19:20:58 2004
>>+# set up 'filehandles' for testing walk_output()
>>+my ($s1,$s2,$s3);
>>+open(my $f1, '>', \$s1);
>>+open(my $f2, '>', \$s2);
>>+open(my $f3, '>', \$s3);
>>
>>
>
>Will those opens work without perlio? Do we care?
>
>
when I ran following, the above code worked. Should it have failed ?
PERLIO=stdio ./perl -Ilib ext/B/t/concise.t
BTW, in 2nd version (patch.concise3) I added a SKIP: {eval "use 5.008";
skip if $@,
a bit superfluous for blead, but...
ALSO, Ive added $walkHandle->can('print'). to walk_output().
one issue remaining here is whether that should carp, and if so, should
it preserve the old
handle. But thats 'coddling' the user, and B::Concise was never for
'beginners'
>
>
>>+my $ref = <<'EORef';
>>+7 <1> leavesub[1 ref] K/REFC,1 ->(end)
>>+- <@> lineseq KP ->7
>>+1 <;> nextstate(main -\d+ concise.t:\d+) v ->2
>>+6 <2> sassign sKS/2 ->7
>>+4 <2> add[t1] sK/2 ->5
>>+- <1> ex-rv2sv sK/1 ->3
>>+2 <$> gvsv(*b) s ->3
>>+3 <$> const(IV 42) s ->4
>>+- <1> ex-rv2sv sKRM*/1 ->6
>>+5 <$> gvsv(*a) s ->6
>>+EORef
>>+
>>+# this test is probably somewhat brittle, despite the qr//
>>+like ($s2, /\Q$ref/, "matches against hardcoded reference output");
>>
>>
>
>I think that like() is getting passed "" as the pattern to check, the *result* of a
>match against $_; probably not what you intended. Even changed to use
>qr//, I don't think you want the \Q, that will make your \d+ literal.
>You need to go through and manually backslash all the $*()[].@ chars.
>
>
Indeed - once I actually added the intended qr//, test broke.
now fixed in my working-copy. Thanks.
>
>
>>+# try more robust test, with brittle lib inclusion
>>+SKIP: {
>>+ use lib '/usr/local/lib/perl5/site_perl/5.8.2/i686-linux-thread-multi';
>>+
>>+ eval "use String::Approx 'amatch'";
>>+ skip("no String::Approx, $@", 1) if $@;
>>+
>>+ my @matches = amatch($s1, $s2);
>>+
>>+ print "@matches\n";
>>+ ok(1, "I dont have it!");
>>+}
>>
>>
>
>Not sure exactly what that's for, but you may want instead:
>use Config;
>use lib $Config::Config{sitelib};
>
>lib.pm is supposed to automatically also add the archname subdirectory
>if it has stuff in it.
>
>
>
It was mostly to test another way.
Since my 'relative' style added "=> \d+" to the end of each line,
String::Approx would produce a predictable edit-distance between
relative and concise outputs.
I gave up when it became obvious that @INC was heavily restricted in
core testing,
but left the test below __END__ to elicit feedback. Thanks for 'taking
the bait' ;-)
I will try your suggestion.
Paul Johnson
> Paul Johnson via RT wrote:
>
> >Jim Cromie said:
> >
> >>3. Tests for above, inc test following pod recipe for using B::Concise
> >>outside O framework.
> >
> >Thanks very much. Presumably I hadn't added any tests because you hadn't
> >added the framework for them yet. Yes, that must be it.
> >
> huh ?
>
> Ok, youre Devel::Cover Dude.
> I see the following in your pod, but I dont see how it relates.
> Could you elaborate ?
I wrote the code and docs for using B::Concise outside of the O
framework, but I didn't write any tests, so I'm very glad that you did.
And yes, it was to help me writing Devel::Cover.
Jim Cromie
> http://rt.perl.org/rt3/Ticket/Display.html?id=24821 >
>
>> The patch adds 3 things:
>>
>> 1. B::Concise::walk_output($fh), with which you can send Concise output
>> to a file, string (ala: open(my $fh,'>',\$string)), etc.
>>
>> 2. B::Concise::add_style(), with which you can add a named style to
>> those already available. This is mostly convenience, but it lets
>> you register new styles, then use them like theyre built-in (
>> including
>> naming them on the command-line, or as args to B::Concise::compile() )
>>
>> 3. Tests for above, inc test following pod recipe for using
>> B::Concise outside O framework.
>> Tested against bleadperl. all OK
>>
This version supersedes others.
I think it adequately addresses all suggestions made here (thanks for
those):
1. walk_output() tests GLOB and ->can('print')
2. regex test vs hardcoded string fixed.
3. appears to just work with PERLIO=stdio
4. String::Approx etal tests were cleaned up to actually run, but
are still below __END__
I didnt see enough value in them wrt what wrong distance
metric results would mean.
the use lib @nearpaths # are a bit hokey
other items
5. added arg-ct tests to set_style, similar to those in add_style
6. added =head2(s) into pod, my additions made the subject drift
without them.
7. printing to walkHandle in a few more places.
8. no comments yet on the pod, I'll *assume* thats a good thing.
[jimc@harpo bleadconcise]$ PERLIO=stdio ./perl -Ilib ext/B/t/concise.t
1..30
ok 1 - require B::Concise
ok 2 - Smallest OP sequence number
ok 3 - Second-smallest OP sequence number
ok 4 - Smallest COP sequence number
ok 5 - -exec option with //=
ok 6 - walk_output() accepts Handle GLOB(0x81478e4)
ok 7 - walk_output() accepts Handle GLOB(0x8147920)
ok 8 - walk_output() rejects bad Handle
ok 9 - walk_output() rejects bad Handle 0
ok 10 - walk_output() rejects bad Handle string
ok 11 - walk_output() rejects bad Handle ARRAY(0x81804c4)
ok 12 - walk_output() rejects bad Handle HASH(0x822f33c)
ok 13 - walk_output() accepts obj that can print
ok 14 - add_style detects insufficient args
ok 15 - add_style detects insufficient [args]
ok 16 - add_style accepts args (stylename => arrayref)
ok 17 - add_style correctly disallows re-adding same style-name
ok 18 - set_style accepts 3 style-format args
ok 19 - set_style rejects bad style-format args
ok 20 - use outside O framework
ok 21 - open to scalar
ok 22 - setup called set_style, add_style, add_callback
ok 23 - walk_output to opened scalar
ok 24 - 3 calls to B::Concise::compile, wo errs
ok 25 - preset style output non-empty
ok 26 - concise style output non-empty
ok 27 - named style output non-empty
ok 28 - preset and named styles are same
ok 29 - concise and named styles are different
ok 30 - matches against hardcoded reference output
Nick Ing-Simmons
And another (more OOish) way is not to check, and just get message
like "cannot find method 'write' via Hugo::File" or whatever.
Nick Ing-Simmons
>Yitzchak Scott-Thoennes via RT wrote:
>
>>On Tue, Jan 06, 2004 at 02:49:59AM -0000, Jim Cromie <perlbug-...@perl.org> wrote:
>>
>>
>>>--- bleadperl/ext/B/t/concise.t Tue Feb 4 14:17:24 2003
>>>+++ bleadconcise/ext/B/t/concise.t Mon Jan 5 19:20:58 2004
>>>+# set up 'filehandles' for testing walk_output()
>>>+my ($s1,$s2,$s3);
>>>+open(my $f1, '>', \$s1);
>>>+open(my $f2, '>', \$s2);
>>>+open(my $f3, '>', \$s3);
>>>
>>>
>>
>>Will those opens work without perlio? Do we care?
>>
>>
>
>when I ran following, the above code worked. Should it have failed ?
>
> PERLIO=stdio ./perl -Ilib ext/B/t/concise.t
"Without perlio" means -Uuseperlio as perl build time, and IO is stdio
as in perl < 5.7.
The \$scalar trick won't work in that case.
I don't think we care - beyond skiping the tests if such a perl is used.
Paul Johnson
Just a couple of apostrophic doc nits:
> +B<add_style> expects args as C<< ($styleName => @stylespec) >> or C<<
> +($styleName => \@stylespec) >>, where @stylespec has 3 strings. It
> +will also die if you attempt to re-add a known style, whether its
it's
> +standard or previously added by you.
> +
> +=head2 add_callback()
> +
> +By calling B<add_callback> and passing references to your callback
> +subroutines, you can populate new variables See L<formatting
> +specifications>, or alter the values of existing ones. These
> +variables are then available for use in the style youve chosen.
you've
Stephen McCamant
JC> Nick Ing-Simmons wrote:
NI-S> And another (more OOish) way is not to check, and just get
NI-S> message like "cannot find method 'write' via Hugo::File" or
NI-S> whatever.
That's also my usual tendency, but I'd just chalk it up to implementor
laziness (notice a distinct lack of any error checking elsewhere in
Concise).
JC> This Latest revision DOES NOT remove the check, instead it adds
JC> another error-check, which more explicitly explains the error that
JC> confused me initially, and started me down this pedantic path.
JC> old error:
JC> Can't locate object method "concise" via package "B::NULL"
JC> new error:
JC> [jimc@harpo concise]$ perl outside.pl -exec Not_There
JC> main::Not_There:
JC> concise_subref(exec, \&main::Not_There) failed: Possible bad function
JC> name (main::Not_There)
JC> [...]
It seems to me that the "Possible" is excessively cautious: if the
user asks for a function by name, and that function doesn't exist,
that's the problem, and we should just give up immediately. I think
the check could go before printing the sub name, for instance.
I've added some more comments on other topics inline in the patch.
JC> diff -ru -x '*.o' bleadperl/ext/B/B/Concise.pm bleadconcise/ext/B/B/Concise.pm
JC> --- bleadperl/ext/B/B/Concise.pm Thu Aug 14 02:02:53 2003
JC> +++ bleadconcise/ext/B/B/Concise.pm Sat Jan 10 15:01:21 2004
JC> @@ -14,10 +14,11 @@
JC>
JC> use Exporter (); # use #5
JC>
JC> -our $VERSION = "0.57";
JC> +our $VERSION = "0.58_01";
Please just increment by cents in whatever version eventually gets
applied. If Concise gets another 40 patches, it will be high time to
declare 1.0 anyway.
JC> our @ISA = qw(Exporter);
JC> our @EXPORT_OK = qw(set_style set_style_standard add_callback
JC> - concise_subref concise_cv concise_main);
JC> + concise_subref concise_cv concise_main
JC> + add_style walk_output);
JC>
JC> [...]
JC>
JC> sub B::OP::concise {
JC> - my($op, $level) = @_;
JC> + my($op, $level, $fh) = @_;
JC> + $fh = $walkHandle unless $fh;
Did you add a call to concise with a $fh argument somewhere else I
missed? Since many other places use the global $walkHandle
unconditionally, it seems cleaner to just do that here too.
JC> if ($order eq "exec" and $lastnext and $$lastnext != $$op) {
JC> my $h = {"seq" => seq($lastnext), "class" => class($lastnext),
JC> "addr" => sprintf("%#x", $$lastnext)};
JC> - print fmt_line($h, $gotofmt, $level+1);
JC> + print $fh fmt_line($h, $gotofmt, $level+1);
JC> }
JC> $lastnext = $op->next;
JC> - print concise_op($op, $level, $format);
JC> + print $fh concise_op($op, $level, $format);
JC> }
JC>
JC> # B::OP::terse (see Terse.pm) now just calls this
JC> @@ -1095,11 +1125,16 @@
JC>
JC> =head1 Using B::Concise outside of the O framework
JC>
JC> -It is possible to extend B<B::Concise> by using it outside of the B<O>
JC> -framework and providing new styles and new variables.
JC> +You can use B<B::Concise> directly, and thereby avoid the compile-only
JC> +operation of O. This allows you, for example, to use the debugger to
JC> +step through B::Concise::compile() itself. When you do this, you can
JC> +alter Concise output by optionally providing new styles, and new
JC> +variables within those styles.
JC> +
JC> +=head2 example: Altering Concise Output
JC>
JC> use B::Concise qw(set_style add_callback);
JC> - set_style($format, $gotofmt, $treefmt);
JC> + set_style($your_format, $your_gotofmt, $your_treefmt);
JC> add_callback
JC> (
JC> sub
JC> @@ -1110,17 +1145,36 @@
JC> );
JC> B::Concise::compile(@options)->();
JC>
JC> +=head2 set_style()
JC> +
JC> You can specify a style by calling the B<set_style> subroutine. If you
JC> have a new variable in your style, or you want to change the value of an
JC> existing variable, you will need to add a callback to specify the value
JC> for that variable.
JC>
JC> -This is done by calling B<add_callback> passing references to any
JC> -callback subroutines. The subroutines are called in the same order as
JC> -they are added. Each subroutine is passed four parameters. These are a
JC> -reference to a hash, the keys of which are the names of the variables
JC> -and the values of which are their values, the op, the level and the
JC> -format.
JC> +=head2 add_style()
JC> +
JC> +You can also create named styles by using B<add_style>, which takes an
JC> +additional styleName argument, and registers that style for later
JC> +selection via B::Concise::compile(). This is handy if you expect to
JC> +use several styles iteratively.
JC> +
JC> +B<add_style> expects args as C<< ($styleName => @stylespec) >> or C<<
JC> +($styleName => \@stylespec) >>, where @stylespec has 3 strings. It
Could you just pick one of these calling styles? It seems silly to
have both.
JC> +will also die if you attempt to re-add a known style, whether its
JC> +standard or previously added by you.
JC> +
JC> +=head2 add_callback()
JC> +
JC> +By calling B<add_callback> and passing references to your callback
JC> +subroutines, you can populate new variables See L<formatting
^
Punctuation here? --/
JC> +specifications>, or alter the values of existing ones. These
JC> +variables are then available for use in the style youve chosen.
JC> +
JC> +The subroutines are called in the same order as they are added. Each
JC> +subroutine is passed four parameters. These are a reference to a
JC> +hash, the keys of which are the names of the variables and the values
JC> +of which are their values, the op, the level and the format.
JC>
JC> To define your own variables, simply add them to the hash, or change
JC> existing values if you need to. The level and format are passed in as
JC> @@ -1128,10 +1182,33 @@
JC> changed or even used.
JC>
JC> To switch back to one of the standard styles like C<concise> or
JC> -C<terse>, use C<set_style_standard>.
JC> +C<terse>, use C<set_style_standard>, or pass the styleName into
JC> +B::Concise::compile.
JC> +
JC> +=head2 Running and Getting Output
JC> +
JC> +To get the output, call the subroutine returned by B<compile>, it will
JC> +print to STDOUT. In addition to the style-options, you can pass one
JC> +or more function names to B<compile>, the function it returns will
JC> +traverse both in order. B<walk_output> allows you to redirect that
JC> +output to a file, or to any object that can print.
JC> +
JC> + open (my $fh, '>', \$concise_output);
JC> + walk_output($fh);
JC> + B::Concise::compile('-concise','funcName')->();
JC> + print "Concise Results: $concise_output\n";
JC> +
JC> +B<compile> will die as follows you if you've asked for a non-existent
JC> +function. (wrapped here for clarity)
JC> +
JC> + main::junk:
JC> + concise_subref(basic, \&main::junk) failed: \
JC> + Possible bad function name (main::junk)
JC> +
JC> +=head1 CAVEATS
JC>
JC> -To see the output, call the subroutine returned by B<compile> in the
JC> -same way that B<O> does.
JC> +This module issues no warnings, all errors are fatal. Use eval to
JC> +prevent premature death.
This phrasing seems a bit alarmist to me: all of the things that cause
dying are things the programmer should be able to prevent in the
course of writing correct code (e.g. passing something that's not a
filehandle). How about:
=head2 Errors
None of the above programming interfaces have error codes; they will
die() on invalid arguments.
JC>
JC> =head1 AUTHOR
JC>
JC> diff -ru -x '*.o' bleadperl/ext/B/t/concise.t bleadconcise/ext/B/t/concise.t
JC> --- bleadperl/ext/B/t/concise.t Tue Feb 4 14:17:24 2003
JC> +++ bleadconcise/ext/B/t/concise.t Sat Jan 10 13:26:31 2004
JC> @@ -6,7 +6,7 @@
JC> require './test.pl';
JC> }
JC>
JC> -plan tests => 5;
JC> +plan tests => 31;
JC>
JC> require_ok("B::Concise");
JC>
JC> @@ -35,3 +35,260 @@
JC> );
JC>
JC> like($out, qr/print/, "-exec option with //=");
JC> +
JC> +######## NEW TESTS ########
Could you think of a more future-proof description than "NEW" to
describe the tests that go under this line?
JC> +
JC> [...]
JC> +
JC> +eval { B::Concise::compile('-exec','non_existent_function')->() };
JC> +like ($@, qr/Possible bad function name \(main::non_existent_function\)/,
JC> + "'-exec' reports non-existent-function properly");
JC> +
JC> +__END__
JC> +
JC> +# these are interesting, but they dont add much
JC> +# in the way of insight. maybe later..
I would be in favor of physically omitting them. Textual approximate
matching doesn't strike me as a good basis for a regression test, and
tests relying on third party modules would also be a lot of trouble.
JC> +SKIP: {
...
JC> + eval "use String::Approx 'amatch'";
...
JC> + eval "use Text::Levenshtein 'distance'";
...
JC> +}
-- Stephen
Jim Cromie
>It seems to me that the "Possible" is excessively cautious:
>
OK. While cleaning up, I added the ability to run compile() on \&subname,
which complements the more command-line oriented 'subname'.
With \&sub args, the $funcname on 1st line of output
is replaced by *B::Concise::compile(CODE(......))*, as
I thought a bare 'CODE(0x84ca04c)' would be unhelpful.
Im open to either
1 - stripping this 'announcement line',
2 - stripping the 0x..... part which makes asub vs asub comparisons
more tedious than it needs to be
3 - making this user-settable (kinda overkill)
Also, I found a bug where callback args were not per docs.
#$_->(\%h, $op, \$format, \$level) for @callbacks;
$_->(\%h, $op, \$level, \$format, $stylename) for @callbacks;
Note also that I added another arg to callback invocation.
It allows callbacks to act in style specific ways. While this might
not be a wise thing for users to do generally, Perl generally
'gives them enough rope'. I used it in the last test to remove
line-number differences from op-nextstate, which allows me
to compare 2 equivalent anonymous subroutines, and report them
as such. More explanation is in the pod in concise.t
I also added B::Concise::_clr_seq(), which resets the sequence.
This is also necessary for the asub vs asub test.
Rest below can be summarized by "OK, all suggestions accepted, acted upon",
along with attached patch.
our $VERSION = "0.58";
>JC> sub B::OP::concise {
>JC> - my($op, $level) = @_;
>JC> + my($op, $level, $fh) = @_;
>JC> + $fh = $walkHandle unless $fh;
>
>Did you add a call to concise with a $fh argument somewhere else
>
Nope. that was 1st whack. now harmonized with other uses.
>Could you just pick one of these calling styles? It seems silly to
>have both.
>
>
OK. only C<< ($styleName => @stylespec) >> now
> ^
>Punctuation here? --/
>
>
Fixed. along with aphostrophe fixes from Paul
>JC> +=head1 CAVEATS
>
>
now changed to =head2 Errors, and reworded, extended
>JC> +######## NEW TESTS ########
>
>Could you think of a more future-proof description than "NEW" to
>describe the tests that go under this line?
>
>
:-} now says 0.58 TESTS
>JC> +# these are interesting, but they dont add much
>JC> +# in the way of insight. maybe later..
>
>I would be in favor of physically omitting them.
>
Gone. I can play with them elsewhere.
> -- Stephen
>
>
>
thnaks
--jimc