Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

Topic/demolish warnings #167

Open
wants to merge 4 commits into
base: master
Choose a base branch
from
Open

Topic/demolish warnings #167

Show file tree
Hide file tree
Changes from 2 commits
Commits
Show all changes
4 commits
Select commit Hold shift + click to select a range
e6ae140
Test that DEMOLISH usage won't emit warnings
jrubinator May 18, 2018
885f2b3
Suppress "DEMOLISH used only once" warnings
jrubinator May 18, 2018
1f0ebe4
Demolish warnings-removal tweaks
jrubinator Jun 27, 2018
a0361cb
Review Tweaks - take two
jrubinator Jul 10, 2018
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
5 changes: 4 additions & 1 deletion lib/Moose/Object.pm
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -78,7 +78,10 @@ sub DEMOLISHALL {

foreach my $class (@isa) {
no strict 'refs';
my $demolish = *{"${class}::DEMOLISH"}{CODE};
my $demolish = do {
no warnings 'once';
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

A comment explaining why this is necessary would be good.

*{"${class}::DEMOLISH"}{CODE};
};
$self->$demolish($in_global_destruction)
if defined $demolish;
}
Expand Down
15 changes: 15 additions & 0 deletions t/bugs/DEMOLISH_warnings.t
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,15 @@
use strict;
use warnings;

use lib 't/lib';
use Test::More tests => 1;

my @warnings;
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We're using Test::Warnings in other tests. Might as well use it here too. That means you need to make it optional with Test::Requires but all the optional tests get run by travis so that's ok.

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'm not entirely clear on how to write this test using Test::Warnings - some of this might be lack of familiarity on my end about when exactly Perl decides to display the warnings: when I replace use, with require, I don't see the DEMOLISH warning. And I don't see how to wrap warnings {} around a use statement - any suggestions?

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

There are a few ways, but the easiest is probably just to load Test::Warnings, load the module in question, and then use had_no_warnings() to do the actual test.

BEGIN {
$SIG{__WARN__} = sub { push @warnings, @_ };
}

use Demolition::OnceRemoved;

is scalar @warnings, 0, "No warnings"
or diag explain \@warnings;
6 changes: 6 additions & 0 deletions t/lib/Demolition/Demolisher.pm
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,6 @@
package Demolition::Demolisher;
use Moose;

sub DEMOLISH { }

1;
9 changes: 9 additions & 0 deletions t/lib/Demolition/OnceRemoved.pm
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,9 @@
package Demolition::OnceRemoved;
use strict;
use warnings;
use Demolition::Demolisher;

my $d = Demolition::Demolisher->new;
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Could we move this into a block in the .t file? I think that'd make it a bit clearer.

$d->DEMOLISH;

1;