There was a discussion about this on #poe, which I thought I'd share:
<+nperez> an isa check every single time something goes through the
filter just seems like overkill...
<+LotR> compared to the work that Filter::XML does?
<+nperez> yes, because it will add up. hold on, writing a benchmark
script
<+LotR> I'm sure it will prove to be nothing compared to parsing an xml
subtree
<+nperez> but what about other filters? Stream? Line?
<+LotR> stream doesn't ever need to generate any errors. nor does line
<+LotR> if you just use those, you can forget about error checking
altogether
* LotR will send the discussion to the list when we're done
<+nopaste> "nperez" at 12.5.72.252 pasted "undef vs isa" (30 lines) at
http://nopaste.snit.ch:8001/10886
<+nperez> take a look
<+nperez> that's a huge difference
<+LotR> but an isa() still is pretty damn fast.
<+nperez> besides, it is less of a rewrite checking for undef.
consider:
you have to now check ref and then isa on every return value
from the filter
<+nperez> what if it isn't a ref that gets returned?
<+nperez> and isa only worked on blessed objects
<+LotR> you're still going to have to check for undef too
<+nperez> works
<+nperez> exactly, so why do the other checks and you can just do the
one
<+LotR> and it is going to be more of a change if you then have to ask
the filter for extra data
<+nperez> not really, implementing an get_error() isn't as profound of
a
change
<+nperez> it is OOB
<+nperez> without you still have checking for undef for errors in a lot
of modules anyhow
<+nperez> if they want to know more info, they then call get_error()
<+nperez> otherwise, they might not care
<+LotR> that "whout you still.." sentence doesn't parse for me
<+LotR> without. duh
<+nperez> sorry, without get_error(), you still have checking for undef
<+LotR> still don't get it. how does checking for undef for errors in
'a
lot of modules' have anything to do with filters?
<+nperez> because right now, the de facto indicator for error is
returning undef
<+LotR> no it isn't
<+LotR> in Filter::HTTPD, an HTTP::Response is returned
<+LotR> in Filter::XML, you get a callback
<+LotR> in Filter::SAXBuilder, you get an Error::Simple object
<+LotR> and I don't know any other filters that return errors right now
<+nperez> okay, thinking a little more general, perl built-ins return
undef on errors and set $!
<+nperez> following that convention can't be /that/ bad
<+LotR> right, but is it guaranteed that you always get the chance to
check the error before an extra error is created (meaning
filters
have to store more than one, and the $! analogy breaks down)
<@Leolo> test it against Scalar::Util::blessed
<+nperez> Leolo: that is a possibility too
<+LotR> Leolo: hmm?
<@Leolo> also, the filter could return undef(), then you ask the filter
->is_error() or ->error_str() or whatever
<@Leolo> call this wrapped in POE::Filter::Fallible
<+LotR> I just don't particulary like the extra API
<@Leolo> s/call/all/
<+nperez> well, it's incomplete as is. Error states are simply
unaccounted for. Most of the modules that I have been looking
through in the past 10 minutes make all sorts of assumptions
on
input
<+nperez> if there is an error, it will just die
<+nperez> or return [ ]
<+nperez> in which case, how do you know there was an error?
<+LotR> you don't. But I like the simplicity of array ref in; array ref
out
<@Leolo> they probably assume they are talking to another
POE::Filter::Foo. in which case there should be no error
<@Leolo> lotr : true
<+nperez> the simplicity fails. If the filter gets into an inconsistent
state due to bad input, but never tells the outside world
what
is going on, that's just poor
<+nperez> returning [] is just too overloaded
<+LotR> plus, if you stack the filters, can you make sure you will just
have had one error if you get out of the filter?
<+LotR> nperez: I'm not saying returning [] on error is a good idea, it
isn't.
<+nperez> there probably needs to be a way to aggregate/bubble errors
up
from stacked filters
<+LotR> I'm saying returning an error object in the array ref out keeps
the simplicity
< imMute> maybe it should return some kind of POE::Filter::Error object
on error
< imMute> and then the wheel thats processing it can deal with the error
< imMute> or the next filter in stackable can just pass it on
<+nperez> LotR: but that adds checking in-band which is a performance
killer
<+nperez> maybe no killer, but detrimental to performance i should say
<+nperez> maybe no killer, but detrimental to performance i should say
< imMute> its better than just suddenly not getting any records from a
filter and not knowing why
<+LotR> nperez: right. so we disagree on how bad such a performance hit
that is
<+nperez> imMute: getting an undef should be a strong idicator of error
in which case you check the error property on the filter
<+nperez> agreed on that point LotR
< imMute> nperez: are we going to standardize the name of that call,
and
what it should return?
* LotR wonders if there are no filters that return undef for reasons
other than error
<+nperez> imMute: that's what we would like to happen, yes
< imMute> personally I think we should standardize the arrayref in,
arrayref out first, because I know there are filters that
dont
follow that
<+nperez> it is standard, if they subclass POE::Filter
<+nperez> and stay "true" to form
< imMute> some dont though
<+nperez> then we should file bugs on rt for them :)
< imMute> i know few of BinGOs' filters subclass POE::Filter
< imMute> and he complained loudly when i added an isa() check to
Stackable
<@BinGOs> all of them do.
<+nperez> I did too
<+nperez> I had to make changes to make the filter stabled
<+nperez> stackable
<+purl> well, stackable is newer than PFX?
<@BinGOs> this is why I was screaming blue fucking murder earlier.
< imMute> maybe its changed since i last messed with filters, its been
a
few months
<+nperez> It is implicit that filters take aref in and spit aref out
<+LotR> explicit in the docs actually
<+nperez> right
<@BinGOs> My take is POE::Filter shouldn't shit out anything they
shouldn't
<+LotR> BinGOs: meaning?
<+purl> somebody said meaning was in the individual, whether a
definition
<@BinGOs> otherwise a world of fucking hurt is going to happen.
<@BinGOs> meaning, don't return anything on a fucking error.
<+LotR> why not?
<@BinGOs> because I will hunt you down like a dog
<+nperez> BinGOs: if there is an error, I'd like to know about it
<@BinGOs> then 'warn'
<+nperez> returning [] is not tenable
<+nperez> the state of the filter may need to be reset
<+nperez> I can't capture warns and adjust accordingly
<@BinGOs> if you are in a POE::Filter::Stackable then everyone may not
be
following the rules.
<+nperez> which is why we are wanting to update the POE::Filter API and
introduce a standard way to report errors
<+LotR> BinGOs: there'll be new rules :)
<+nperez> heh
<@BinGOs> I am not the bad guy here >:)
<+nperez> BinGOs: if you want patches to your modules after the change,
I'll help, promise. Because you have a shit load and I don't
blame you for beign resistant to change ;)
<+LotR> BinGOs: oh no? so what happens if Filter::IRC gets a line that
doesn't parse?
<@BinGOs> it warns
<+LotR> which isn't very nice
<@BinGOs> and b). I am not using POE::Filter::IRC these days.
<+LotR> Filter::IRCD then
<@BinGOs> and c). yes, POE::Filter::IRCD warns
<+nperez> warnings disappear into the aether and don't control
autocorrecting procedures
<+LotR> BinGOs: which you can't handle very nicely.
<@BinGOs> Okay, you have a POE::Filter::Stackable
< imMute> what does PCI use for the client then?
<@BinGOs> the first filter produces an error, what happens next ?
<@BinGOs> ( you have four filters in the chain )
<+nperez> stackable checks the error property, sets its own error
property and returns undef
<+nperez> skipping the other three
<@BinGOs> do you produce an event at the end ?
<+LotR> or
<+LotR> it sticks an error object in the listref out. (and probably
skips
the other three filters)
< imMute> BinGOs: no
<+LotR> BinGOs: filters don't know anything about POE events. I want to
keep it that way
<@BinGOs> PCI uses Filter::IRCD and POE::Filter::IRC::Compat in a
stackable.
< imMute> BinGOs: stackable doesnt produce output unless the last
filter
returns something
<@BinGOs> SO what happens to your error then ?
<+nperez> returning undef should be an indicator to check the error
property on the filter
<+LotR> BinGOs: read the scrollback? we've been over this and haven't
come to an agreement. nperez and I have different views on how
to
do it
<+LotR> but we agree there should be some error reporting in filters
<@BinGOs> Hell, I'll work around whatever you come up with anyways >:)
<+nperez> haha
<@BinGOs> difficult audience.
<+nperez> Ultimately is up to dngor about an API change
<+nperez> we are wanting to do something rather fundamental
<+nperez> especially with a 1.0 approaching
<@BinGOs> this is why I am stomping my little feet.
<+nperez> If it's broken, it's broken. Bitching how much the fix hurts
isn't going to make it go away :)
<+LotR> another reason not to do an ->get_error() method. no need to
change old filters who don't care about error reporting
<+nperez> if they don't care about error reporting, they aren't
checking
their inputs enough ;)
<@BinGOs> I am a team player and have a 'Can do attitude'
<@BinGOs> the whole POE::Filter sub-class thing was no problem to me.
<+nperez> BinGOs: we aren't interviewing you here. You are more than
welcome to be the devil's advocate and general asshole. It
helps :)
<@BinGOs> Yes, I am playing devil's advocate
<@BinGOs> and yes, I am an asshole.
<+nperez> You represent a large swath of incumbent modules. Your
opinion
counts
<@BinGOs> I always have an eye on backwards compatibility.
<@perigrin> to watch it slip away
<+nperez> That is certainly something to consider. LotR's idea is
probably better for backwards compat
<@Leolo> backwards compatible with "do nothing useful" is easy
