Hi Steve, sorry for the late response (not that I have a lot to say except mostly agreeing with what you've written).
On Thu, 3 May 2012 18:49:15 -0400 Steve Scaffidi <[email protected]> wrote: > This weekend, I plan on spending a little time on I::P::C and so here > is my response to Shlomi's commentary: > > On Tue, Apr 10, 2012 at 10:39 AM, Shlomi Fish <[email protected]> wrote: > > In > > https://github.com/Hercynium/IPC-Pipeline-Composable/blob/master/lib/IPC/Pipeline/Composable.pm > > : > > > > sub ipc_pipeline { > > my ($class, @procs) = > > !eval { $_[0]->isa(__PACKAGE__) } ? (__PACKAGE__, @_) : > > blessed($_[0]) ? (__PACKAGE__, @_) : > > @_; > > > > Seems like $procs[0] will always be $_[0] except for the last case. > > Furthermore, the two first results are identical and should be merged. Maybe > > this code is too clever for its own good. > > I agree. I initially did it for convenience in prototyping and > testing. In the redesign, this will stop being a chimera sub (my term > for a sub that serves simultaneously as class method, object method, > and function) and end up being purely a function. It will actually > live in I::P::C::Functions, much like the exported subs in > File::Spec::Functions. Heh, "Chimera sub" - I like it. It's good that you get rid of this anti-pattern. > > > Later on in the same function you have: > > > > return $class->new( > > procs => [ map { > > # IPC::Process object can be used as-is > > eval { $_->isa("${class}::Process") } ? $_ : > > # IPC object needs its processes extracted > > eval { $_->isa($class) } ? ($_->procs) : > > # If it's a non-ref, try to stringify and use that as the process spec > > ! ref($_) ? ${ \"${class}::Process" }->new(cmd_str => $_) : > > # Initialize a Process object from an arrayref > > reftype($_) eq 'ARRAY' ? ${ \"${class}::Process" }->new(cmd => > > shift(@$_), args => $_) : > > # Initialize a Process object from a subref > > reftype($_) eq 'CODE' ? ${ \"${class}::Process" }->new(cmd => $_) : > > # unknown argument? DIE! > > die "unhandled type passed to ipc_pipeline!\n"; > > # TODO: handle placeholders for processes > > } @procs ]); } > > > > I think all this is too complex to be placed in map when it's not extracted. > > Furthermore, I also think that you risk cloberring "$_", and I would suggest > > assigning it to a lexical variable. > > I agree this needs to be redesigned. It should go away completely when > I make everything OO and simply dispatch actions based on the ->isa(). > In that case it will just be a hash, and that design should also allow > for some interesting opportunities for extensibility. > OK, good. > > Finally, why are you using ${ \"..." } instead of just "..."? > > Ah, good catch! Each $class var there was originally __PACKAGE__ and > then I realized how dumb that was and replaced it wil something only > slightly less dumb :) > :-). > > Moving on: > > > > [CODE] > > > > # allow exported subs to be called as functions, or object/class methods. > > sub __func_meth_args { > > return > > !eval { $_[0]->isa(__PACKAGE__) } ? (__PACKAGE__, @_) : # function call > > blessed($_[0]) ? (__PACKAGE__, @_) : # object method call > > @_; # class method call > > } > > > > [/CODE] > > > > This seems like duplicate code to what I've commented about before, and I > > believe that a CGI.pm-like subroutines that are callable either as methods > > or > > as non-OOP functions and procedures is not a good idea. If you do want to > > provide a procedural wrapper, then you should put it in another namespace. > > Agreed! > > > [CODE] > > # This is so non-portable... I can't find a CPAN dist for this??? > > sub __proc_path_from_fh { > > return > > $OSNAME =~ /Linux/i ? "/proc/self/fd/".fileno(shift) : > > $OSNAME =~ /MacOS|darwin/i ? "/dev/fd/".fileno(shift) : > > return; > > } > > [/CODE] > > > > You have a small amount of duplicate code there and you're also doing > > "return return;", which doesn't make a lot of sense. Since this code also > > only supports Linux and Mac OS X, I would suggest taking a look at the > > source for GNU Bash and/or similar portable shells and see how they are > > doing it. You may also wish to consider extracting this functionality into > > its > > own self-contained CPAN distribution. > > Luckily, I think I've decided to *not* support this functionality > since I've learned that doing so cross-platforms would be a nightmare > I simply don't want to face right now, > I see. > > That's it for now, but I haven't reviewed the code thoroughly. As a general > > impression, I would say that it seems a little too clever for my taste. > > I'm guessing that's why it isn't yet on the CPAN! ;-P (/me invokes his > sense of hubris to make this joke) OK. Regards, Shlomi Fish -- ----------------------------------------------------------------- Shlomi Fish http://www.shlomifish.org/ List of Portability Libraries - http://shlom.in/port-libs Chuck Norris can read Perl code that was RSA encrypted. Please reply to list if it's a mailing list post - http://shlom.in/reply . _______________________________________________ Boston-pm mailing list [email protected] http://mail.pm.org/mailman/listinfo/boston-pm
