[Prophet] A few random questions on the source
Gianni Ceccarelli
dakkar at thenautilus.net
Mon Aug 18 18:16:11 EDT 2008
Hi all!
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.
Thanks in advance for any clarification / correction.
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?
+
=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
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',
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";
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?
$_[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?
# "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)
+
# 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
$self->type($self->primary_commands->[-2]);
}
}
@@ -403,6 +408,8 @@
}
}
+# why not using the MooseX::AttributeHelpers::MethodProvider::Hash 'clear' method?
+
=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?
} 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
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;
$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
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 '/'?
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)
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
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"?
=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?
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)
-------------- next part --------------
A non-text attachment was scrubbed...
Name: signature.asc
Type: application/pgp-signature
Size: 197 bytes
Desc: not available
Url : http://lists.bestpractical.com/pipermail/prophet/attachments/20080819/df3b1df4/attachment.pgp
More information about the Prophet
mailing list