[Prophet] A few random questions on the source

[Shawn M Moore]

On Tue, Aug 19, 2008 at 10:56:29PM +0200, Gianni Ceccarelli wrote:
> On 2008-08-19 Shawn M Moore <sartak at bestpractical.com> wrote:
> > Thanks for all these good (and sometimes (rightfully) harsh) comments!
> 
> Hmmm. I didn't actually intend for any of them to be harsh, they may
> be seem that way because they are just short comments??? I'll try to be
> a bit more verbose, so as not to sound curt/harsh.

Harsh in the sense that you're keeping us honest by being strict about
questionable code. :) You weren't mean at all.

> > 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.
> 
> Yes, it's clearly intended to be called that way, but Prophet::CLI
> calls it as ??Prophet::App->require;??, which lacks the second
> parameter, thus exiting without actually requiring anything. Or, more
> probably, I'm missing something obvious.

Ah, this is a vestige of our use of UNIVERSAL::require. I've removed
this line because it wasn't doing anything anyway, as you correctly
pointed out. :)

> > >      } elsif ( $foreign_class->isa('Prophet::Record') ) {
> > > -
> > > +# uh? nothing happens?
> > 
> > This elsif clause avoids the exception below.
> 
> Yes, my question was poorly stated. Let me rephrase: what should
> happen when I
> ??My::Record->register_reference('stuff',My::Other::Record)???
> Currently, nothing seem to happen, not even a warning. Either the call
> is meaningless, or I'm missing the implementation.

We currently support one-to-many relations, but not one-to-one
relations. When we add support for this, this is where the code will
go. I've made it throw a different exception for now, to indicate why we
have this useless codepath. :)

> > > +    # 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.
> 
> From perlfunc:
> 
> > When assigning to a list, if LIMIT is omitted, or zero, Perl
> > supplies a LIMIT one larger than the number of variables in the
> > list, to avoid unnecessary work.
> 
> Note the "one larger". ??my ($schema,$url)=split
> /:/,'prophet:file:///tmp/stuff'?? puts 'prophet' in $schema and 'file'
> in $url.

Ugh. Damnit Perl! :)

I've made this use a more clear regex match.

> > > +    # 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.
> 
> Right, "unnecessary" is a much better term for what I meant.

Yeah. I'll just leave it for now. :)

Anyway, thanks for your cleanup suggestions.

If you're IRC inclined, we hang out in #prophet on irc.freenode.org.

Shawn