Bug #34779 for POE: [PATCH] POE::Data::Envelope Integration Patch

> > <URL: http://rt.cpan.org/Ticket/Display.html?id=34779 > > > Hi, Nick! Now that it's the weekend, I have some time to look at your > patch. > > It looks like a lot of tests have been altered to strip the blessing > from results. Unfortunately tests like this don't test the actual data > but rather an altered copy. I think that's bad form: > > is_deeply( > - $next, [ "((($compare)))" ], > + [@$next], [ "((($compare)))" ], > "map filter get_one() returns ((($compare)))" > ); > > This is significantly better, assuming it works: > > is_deeply( > - $next, [ "((($compare)))" ], > + $next, bless([ "((($compare)))" ], 'POE::Data::Envelope'), > "map filter get_one() returns ((($compare)))" > ); > > You've broken many of the ref() tests. ref([@$record]) is always equal > to 'ARRAY' because you've wrapped the record in an arrayref! Worse, > @$record will be a fatal error if ref($record) isn't based on ARRAY. > > my $records = $filter->get([ $get_request->as_string ]); > - is(ref($records), 'ARRAY', 'simple get: get() returns list of > requests'); > + is(ref([@$records]), 'ARRAY', 'simple get: get() returns list of > requests'); > is(scalar(@$records), 1, 'simple get: get() returned single request'); > > As above, I'd like to see $records tested for what it is. > > ... and that's all I have time for today. I'll see about the > filter/wheel changes tomorrow. >

> > <URL: http://rt.cpan.org/Ticket/Display.html?id=34779 > > > On Wed Apr 09 02:49:23 2008, NPEREZ wrote: > > I agree that the tests should verify that PDE behaves like plain old > arrays for backward compatibility, but the spirit of many existing tests > has been violated. I think the array-ness tests should be added, and > the existing tests changed so their purposes remain intact. > > I'm looking at the modified POE::Filter::Stream. That's the most > trivial filter, so your changes really stand out. > > The following are half-informed comments based on an incomplete > understanding of your changes. Let me know where/how I get it wrong. > > Something's bugging me about the way POE::Data::Envelope objects hold > data but are treated separate from it. > > This puts a constraint on filters: they must not pass two PDE objects > amongst themselves, otherwise information is lost. > > So if a filter receives something which triggers two PDE objects, it > will need to hold one of them until the next get_one() call. > > If two different PDE objects arrive via get_one_start() then the filter > must not combine them: > > $f->get_one_start( PDE->new(...one...) ); > $f->get_one_start( PDE->new(...two...) ); > while (1) { > my $one = $f->get_one(); > last unless @$one; > ... > } > > In the above case, POE::Filter::Stream will lose the information encoded > in the first PDE object. The second one will overwrite it. > > ... > > The get_one() call may not come for a while. For example, the line may > be quiet, or select_pause_read() may have been called until the second > PDE object arrives. In the latter case, there's a deadlock. > > sub get_one_start { > if (blah blah) { > # push two PDE objects into the stream > } > } > > So the wheels must, upon receipt of a PDE object, drive another filter > get cycle just in case there's a pending object in the stack. > > ... > > The general get_one_start() logic may be something like this: If the > new PDE is the same as the last one, then combine the data with the last > one's. Otherwise push the PDE onto the stream buffer. > > A half-assed prototype: > > sub get_one_start { > my ($self, $data) = @_; > if (ref($data) eq ref($self->{buffer}[-1])) { > push @{$self->{buffer}[-1]}, @$data; # or something similar > } > else { > push @{$self->{buffer}}, $data; > } > } > > ... > > clone() clones the PDE object but not the data. I'm not sure that's a > good idea, as it could be bad for PDE objects to bleed across filters. > > This could resolve itself if PDE objects were the buffer. > > ... > > POE::Filter's get() is not PDE aware. If get_one() returns a PDE > object, get wraps it up in an anonymous array. > > ... > > Should get_pending() return the current buffer wrapped in the current > PDE environment? POE::Wheel::Whatever feeds get_pending()'s return > value to get_one_start() for the new filter. > > ... > > Probably other questions, but it's 4am. G'night! >