-
Notifications
You must be signed in to change notification settings - Fork 576
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.
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
added progress notifications to Mojo::IOLoop::Subprocess #1286
Conversation
lib/Mojo/IOLoop/Subprocess.pm
Outdated
@@ -10,6 +10,7 @@ use Storable; | |||
has deserialize => sub { \&Storable::thaw }; | |||
has ioloop => sub { Mojo::IOLoop->singleton }, weak => 1; | |||
has serialize => sub { \&Storable::freeze }; | |||
has _progress_writer => undef, weak => 1; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We don't use private attribute accessors. Why do you need to weaken the reference?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I weakened the reference thinking that would allow the pipe to disappear once ->run
completes (but before $subprocess
goes out of scope), and thus save on resources. Is there something wrong with that thinking?
lib/Mojo/IOLoop/Subprocess.pm
Outdated
$snippet =~ s/\A$length\-//; | ||
my $args = $self->deserialize->($snippet); | ||
$self->emit(progress => @$args); | ||
} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This block is in need of some golfing, it seems way too verbose. 😉
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
pos
can replace the length($length) + 1
my ($length, $s) = (($progress_buffer =~ m/\A(\d+)\-/gc), pos($progress_buffer));
defined $length or last;
length($prog_buffer) >= ($length += $s) or last;
my $args = $self->deserialize->(substr substr($progress_buffer, 0, $length, ''), $s);
$self->emit(progress => @$args);
Squash your commits. |
lib/Mojo/IOLoop/Subprocess.pm
Outdated
# Child | ||
return $self->$parent("Can't fork: $!") | ||
unless defined(my $pid = $self->{pid} = fork); | ||
unless ($pid) { | ||
close $progress_reader; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Why does $progress_reader
need to be closed manually, but not $reader
?
lgtm 👌 |
Summary
Added 'progress' method and event to Mojo::IOLoop::Subprocess, in accordance to kraih's specifications. Includes test, but no documentation just yet.
Motivation
This pull-request comes after an issue for its implementation was written in the mojo project
References
issue #1285