[Prophet] A few random questions on the source
Shawn M Moore
sartak at bestpractical.com
Tue Aug 19 12:32:10 EDT 2008
On Tue, Aug 19, 2008 at 12:16:11AM +0200, Gianni Ceccarelli wrote:
> Hi all!
Hello Gianni!
> I'm new to Prophet, having discovered it thanks to obra's talk at
> YAPC::EU in Copenhagen.
>
> I've read most of the source code, and I have a few questions /
> comment. I've listed them below, in the form of a diff: I actually put
> comments in the source while I was reading it, and then 'svn diff'
> produced the following output.
Cool. That's not a bad way of doing it.
> Thanks in advance for any clarification / correction.
Thanks for all these good (and sometimes (rightfully) harsh) comments!
Here we go!
> Index: lib/Prophet/Config.pm
> ===================================================================
> --- lib/Prophet/Config.pm (revision 15237)
> +++ lib/Prophet/Config.pm (working copy)
> @@ -107,6 +107,8 @@
>
> =cut
>
> +# prophet_config_file is not used anymore?
> +
prophet_config_file looks like it was deprecated in favor of
app_config_file. The commented-out method can be removed.
> =head2 prophet_config_file
>
> The file which controls configuration for all Prophet apps. C<$HOME/.prophetrc>.
> Index: lib/Prophet/Server.pm
> ===================================================================
> --- lib/Prophet/Server.pm (revision 15237)
> +++ lib/Prophet/Server.pm (working copy)
> @@ -26,6 +26,9 @@
> domain => 'local',
> );
> } else {
> + # this warning is sligthliy wrong: $publisher might be undef because
> + # require died, so what's actually missing is the whole
> + # Net::Rendezvous::Publish module
Yeah, this is intentional. We don't want to make
Net::Rendezvous::Publish a hard dependency, because we figure most users
won't need to install it because they won't use this feature.
However, if you do want the feature, you can install it and Prophet
becomes a little smarter.
I'm not familiar with the module itself, so the slightly odd code might
be an artifact of Net::Rendezvous::Publish's interface.
> warn
> "Publisher backend is not available. Install one of the ".
> "Net::Rendezvous::Publish::Backend modules from CPAN.";
> @@ -60,6 +63,7 @@
> my $content = $self->handle->read_file($repo_file);
> return unless length($content);
> return $self->_send_content(
> + # custom MIME types must begin with x-
> content_type => 'application/prophet-needs-a-better-type',
Yeah, we'll be changing this one soon.
Jesse: how's application/x-prophet?
> content => $content
> );
> @@ -166,6 +170,8 @@
> my %args = validate( @_, { content => 1, content_type => 1 } );
> print "HTTP/1.0 200 OK\r\n";
> print "Content-Type: " . $args{'content_type'} . "\r\n";
> + # if $args{content} has character semantics, the reported length will be
> + # wrong: it has to be in bytes
> print "Content-Length: " . length( $args{'content'} ) . "\r\n\r\n";
Skipping this one, I don't know enough about encoding and the HTTP
protocol :)
> print $args{'content'};
> return '200';
> Index: lib/Prophet/CLI.pm
> ===================================================================
> --- lib/Prophet/CLI.pm (revision 15237)
> +++ lib/Prophet/CLI.pm (working copy)
> @@ -28,6 +28,7 @@
> lazy => 1,
> handles => [qw/handle resdb_handle config/],
> default => sub {
> + # Prophet::App::require($pack_name) returns without doing anything: what's this call for?
require just loads the class up. Perl's own require builtin has a deficiency:
Perl doesn't understand that require "Foo::Bar" should load Foo/Bar.pm.
So we have a number of workarounds to get the same semantics, this
require method being one of them.
Prophet::App::require is actually supposed to be invoked as a method
(so, Prophet::App->require($pack_name)). If you were poking at this
manually that's why it would appear to do nothing.
> $_[0]->app_class->require;
> return $_[0]->app_class->new;
> },
> @@ -246,6 +247,7 @@
> my @primary;
> push @primary, shift @ARGV while ( $ARGV[0] && $ARGV[0] !~ /^--/ );
>
> + # is it a good idea to do this here? why not factor it out to a munge_primary_command method?
Yes, good call.
I've been slowly working on a refactor of command dispatch so that it
uses a new module Path::Dispatcher (not on CPAN yet, it's still
incomplete). Hopefully it will get rid of most of this confusing code.
> # "ticket show 4" should DWIM and "ticket show --id=4"
> $self->set_arg(id => pop @primary)
> if @primary && $primary[-1] =~ /^(?:\d+|[0-9a-f]{8}\-)/i;
> @@ -279,6 +281,8 @@
>
> no warnings 'uninitialized';
>
> + # comparators are split even if we are setting args instead of props (and are thus lost)
> +
We don't use comparators for args, so this shouldn't be a problem in
practice. In any case, it's an easy check for whether we're looking at
args or props, which should be added here.
> # but wait! does the value look enough like a comparator? if so,
> # shift off another one (if we can)
> if ($val =~ /^(?:$cmp_re)$/ && @ARGV && $ARGV[0] !~ /^--/) {
> @@ -336,6 +340,7 @@
> if ( my $type = $self->delete_arg('type') ) {
> $self->type($type);
> } elsif($self->primary_commands->[-2]) {
> + # this dies if we have too few items
This code is very DWIMmy (it's here to support "./sd ticket update" type
syntax), but this'll go away once that dispatch refactor finishes.
> $self->type($self->primary_commands->[-2]);
> }
> }
> @@ -403,6 +408,8 @@
> }
> }
>
> +# why not using the MooseX::AttributeHelpers::MethodProvider::Hash 'clear' method?
Agreed.
> +
> =head2 clear_args
>
> Clears all of the current object's set arguments.
> Index: lib/Prophet/Record.pm
> ===================================================================
> --- lib/Prophet/Record.pm (revision 15237)
> +++ lib/Prophet/Record.pm (working copy)
> @@ -114,7 +114,7 @@
> @args
> );
> } elsif ( $foreign_class->isa('Prophet::Record') ) {
> -
> +# uh? nothing happens?
This elsif clause avoids the exception below.
> } else {
> die "Your foreign class ($foreign_class) must be a subclass of Prophet::Record or Prophet::Collection";
> }
> Index: lib/Prophet/Replica.pm
> ===================================================================
> --- lib/Prophet/Replica.pm (revision 15237)
> +++ lib/Prophet/Replica.pm (working copy)
> @@ -127,6 +127,8 @@
> my $self = shift;
> my $url = shift;
>
> + # shouldn't that be split /:/, $url,2 ?
> + # just in case $url contains more than one colon
Perl is smart enough to handle this correctly when $url contains
multiple colons. For split (and only for split!) it looks at how many
variables you're sticking the result into and does what you want.
> my ($scheme, $real_url) = split /:/, $url;
> my $type_map = $Prophet::Replica::REPLICA_TYPE_MAP->{$scheme};
> $real_url = $url if $type_map->{keep_scheme};
> @@ -300,6 +302,7 @@
> $self->record_changes($changeset);
>
> my $state_handle = $self->state_handle;
> + # uh? weird way of boolifying
> my $inside_edit = $state_handle->current_edit ? 1 : 0;
I don't quite see how this is weird. Though it is perhaps unnecessary.
>
> $state_handle->begin_edit(source => $changeset) unless ($inside_edit);
> @@ -346,6 +349,7 @@
> }
> # Otherwise, if the we have a merge ticket from the source, we don't want the changeset
> # if the source's sequence # is >= the changeset's sequence #, we can safely skip it
> + # are changesets cumulative? that is, cs 5 includes cs 4? otherwise, I'm not sure this control makes sense
Changesets are not directly cumulative, only indirectly. Changesets are
_ordered_ so that if you're about to apply changeset 5, you will have
already applied changeset 4.
> elsif ( $self->last_changeset_from_source( $changeset->original_source_uuid ) >= $changeset->original_sequence_no ) {
> $self->log("\t - We have seen this or a more recent changeset from remote.");
> return 1;
> Index: lib/Prophet/App.pm
> ===================================================================
> --- lib/Prophet/App.pm (revision 15237)
> +++ lib/Prophet/App.pm (working copy)
> @@ -23,6 +23,7 @@
> my $self = shift;
> return $self->handle->resolution_db_handle
> if $self->handle->resolution_db_handle;
> + # ugly... what happens if PROPHET_REPO (or the stringification of Path::Class::Dir) ends with '/'?
Yeah, this probably wants a cleanup.
> my $root = ($ENV{'PROPHET_REPO'} || dir($ENV{'HOME'}, '.prophet')) . "_res";
> my $type = $self->default_replica_type;
> return Prophet::Replica->new({ url => $type.':file://' . $root });
> @@ -55,6 +56,7 @@
> sub _load_replica_types {
> my $self = shift;
> my $replica_class = blessed($self)."::Replica";
> + # shouldn't this be "^\Q$replica_class\E::.*::"? (no parens, anchor at the beginning)
Yes.
> my $except = $replica_class."::(.*)::";
> Module::Pluggable->import( search_path => $replica_class, sub_name => 'app_replica_types', require => 0, except => qr/$except/);
> for my $package ( $self->app_replica_types) {
> Index: lib/Prophet/Resolver/FromResolutionDB.pm
> ===================================================================
> --- lib/Prophet/Resolver/FromResolutionDB.pm (revision 15237)
> +++ lib/Prophet/Resolver/FromResolutionDB.pm (working copy)
> @@ -27,6 +27,7 @@
> $answer_map{$key} ||= $answer;
> $answer_count{$key}++;
> }
> + # this is random in case of a tie
Yeah, we should probably prompt in case of a tie. This seems to do fine
with picking an arbitrary winner though.
> my $best = ( sort { $answer_count{$b} <=> $answer_count{$a} } keys %answer_map )[0];
>
> my $answer = $answer_map{$best};
> Index: lib/Prophet/CLI/RecordCommand.pm
> ===================================================================
> --- lib/Prophet/CLI/RecordCommand.pm (revision 15237)
> +++ lib/Prophet/CLI/RecordCommand.pm (working copy)
> @@ -21,6 +21,7 @@
> isa => 'Prophet::Record',
> );
>
> +# might this be better called "_get_record_object"?
Yes. Instantiating an object is probably newer than the method's name
which never got updated.
> =head2 _get_record_class [{ type => 'type' }]
>
> Tries to determine a record class from either the given type argument or
> Index: lib/Prophet/CLI/Command/Config.pm
> ===================================================================
> --- lib/Prophet/CLI/Command/Config.pm (revision 15237)
> +++ lib/Prophet/CLI/Command/Config.pm (working copy)
> @@ -11,6 +11,7 @@
> print "Configuration:\n\n";
> my @files =@{$config->config_files};
> if (!scalar @files) {
> + # the name of the default config file is ovverridable by subclasses of Prophet::App, isn't it? should this message take it into account?
Yeah, we should use @files, if only to report "here's where I looked,
maybe one of these makes sense to you".
Also, the error message should be in its own method so that subclasses can
override it.
> print "No configuration files found. ".
> " Either create a file called 'prophetrc' inside of ". $self->handle->fs_root ." or set the PROPHET_APP_CONFIG environement variable.\n\n";
> return;
>
>
> --
> Dakkar - <Mobilis in mobile>
> GPG public key fingerprint = A071 E618 DD2C 5901 9574
> 6FE2 40EA 9883 7519 3F88
> key id = 0x75193F88
>
> - BIG FIDO?
> - "Yes?"
> - HEEL.
> -- (Terry Pratchett, Men at Arms)
Thanks a lot for all of your comments, Gianni. Tonight I'll go through
this mail I just wrote and clean up the code.
Hope you enjoy Prophet!
Shawn
More information about the Prophet
mailing list