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. > 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. > 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, > 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) -- -- Steve Scaffidi <[email protected]> _______________________________________________ Boston-pm mailing list [email protected] http://mail.pm.org/mailman/listinfo/boston-pm
