Fix up PPI::Token::insert_{after,before}.

[hartzell]

I stumbled across this while trying to make
Dist::Zilla::Plugin::PkgVersion work better with perlcritic. More
info here.

The comments for PPI::Token::insert_{after,before} say that they may
only insert statements, but the code inexplicably requires
PPI::Structures.

Requiring a Statement a) seems reasonable to me (but...) and b) makes my use case easier.

This commit makes the code match the comment.

The tests in t still pass.

I'm hoping for feedback from folks wise.

PPI::Statement's code and comment require that a Statement be inserted. PPI::Structure's code and comments require a Structure.

PPI::Token is the only one who's code and comments don't match. It's possible that a better fix is to fix the comment.

[hartzell]

On further study I see that the code does not match the comment in another sense: the comment would require a "non-significant" toekn but the code doesn't check for significance.

Changing the elseif to this:

        } elsif ( $Element->isa('PPI::Token') and ! $Element->significant ) {

causes t/ppi_element.t to fail thusly.

inky:PPI hartzell$ env PERL5LIB=`pwd`/local/lib/perl5 prove -l t/ppi_element.t 
t/ppi_element.t .. 1/58
#   Failed test 'insert_after actually inserts'
#   at t/ppi_element.t line 148.
#          got: 'print 'Hello World';'
#     expected: 'print 'Hello World'foo;'
# Looks like you failed 1 test of 58.
t/ppi_element.t .. Dubious, test returned 1 (wstat 256, 0x100)
Failed 1/58 subtests

Test Summary Report
-------------------
t/ppi_element.t (Wstat: 256 Tests: 58 Failed: 1)
  Failed test:  39
  Non-zero exit status: 1
Files=1, Tests=58,  0 wallclock secs ( 0.04 usr  0.00 sys +  0.10 cusr  0.01 csys =  0.15 CPU)
Result: FAIL

Here's the failing test:

INSERT_AFTER: {
        my $Document = PPI::Document->new( \"print 'Hello World';" );
        isa_ok( $Document, 'PPI::Document' );
        my $string = $Document->find_first('Token::Quote');
        isa_ok( $string, 'PPI::Token::Quote' );
        is( $string->content, "'Hello World'", 'Got expected token' );
        my $foo = PPI::Token::Word->new('foo');
        isa_ok( $foo, 'PPI::Token::Word' );
        is( $foo->content, 'foo', 'Created Word token' );
        $string->insert_after( $foo );
        is( $Document->serialize, "print 'Hello World'foo;",
                'insert_after actually inserts' );
}

The element stored into $foo is signficant, arguably it doesn't make sense to be allowed to insert it there. An insignificant Element (e.g. some whitespace) seems allowable though.

Feedback?

[wchristian]

Sorry for taking so long to get back to this. I'm tempted to include the PR as it is for 1.222, however looking over the code i find myself confused and have created issue #93 to try and clear up some questions about it. If @adamkennedy doesn't answer in the next week i will include this PR for 1.222, and if no answer is forthcoming later on, i will simply remove these restrictions.

[adamkennedy]

I'll find some time to day to check into this, it's possible the comments
or the code are wrong.

Nesting rules say that statements can't have a statement as a direct child,
and structures can't have a structure as a child.

We need to make sure that remains enforced.

Adam
On Nov 2, 2014 8:47 AM, "Christian Walde" [email protected] wrote:

Sorry for taking so long to get back to this. I'm tempted to include the
PR as it is for 1.222, however looking over the code i find myself confused
and have created issue #93 #93
to try and clear up some questions about it. If @adamkennedy
https://github.com/adamkennedy doesn't answer in the next week i will
include this PR for 1.222, and if no answer is forthcoming later on, i will
simply remove these restrictions.

—
Reply to this email directly or view it on GitHub
#79 (comment).

[wchristian]

After dumpering a bit it seems to make sense. Do i understand it right that the rules are like this, and the behavior of the insert functions follows from these?

• The parent of a Statement is always a Document or a Structure.
• The parent of a Structure is always a Statement.
• The parent of a Token is always a Statement.

If i understand that right, then the best fix would be to ensure that behavior is always enforced and more importantly, that this is documented usefully, with notes pointing out that one might need to insert after/before the parent instead of the element one is working on.

[wchristian]

Upon further reading, i see this is documented here: https://metacpan.org/pod/PPI#The-Document-Statement-and-Structure

The wording is a bit ambiguous for my taste, but the insert method documentation absolutely should point there and explain things.

[adamkennedy]

Is be happy to have the wording cleaned up, I have too deep an
understanding of the internals to wrote legible explanations for some
things.

This is probably one of them.

Adam
On Nov 2, 2014 10:43 AM, "Christian Walde" [email protected] wrote:

Upon further reading, i see this is documented here:
https://metacpan.org/pod/PPI#The-Document-Statement-and-Structure

The wording is a bit ambiguous for my taste, but the insert method
documentation absolutely should point there and explain things.

—
Reply to this email directly or view it on GitHub
#79 (comment).

[wchristian]

Thanks Adam, i've amended #93 to make it a proper bug ticket, pointing at this to explain what needs to be done. So for this ticket then there remains only the task of figuring out whether the insert code in Token is correct with respect to its comment.