xsawyerx / PearlBee

Blogging system in Modern Perl
6 stars 2 forks source link

Improve static support #20

Closed book closed 8 years ago

book commented 8 years ago

This PR is related to xsawyerx/PearlBee#19.

Summary of changes:

The remaining work is to fix the templates to use the various uri methods.

andrewalker commented 8 years ago

The code seems very reasonable to me.

It is a little awkward to see a global variable, but I don't know if there is a better way to do it.

Another thing that got me curious is the way the dashboard classes are loaded now. Is there any side effects in skipping import on those, and loading them on runtime instead of compile time? I haven't tested them myself, but just wondering if there is some magic Dancer2 might do that is skipped now because of that...

Otherwise, I'd say :shipit:

book commented 8 years ago

I don't like the global variable either, but the only solution I found that did not involve one was to pass the settings to the url method. Doing something like [% post.url_for( settings ) %] in every template is much more of a pain to do than [% post.url %]. So I think the global variable is an acceptable compromise.

book commented 8 years ago

I've been trying to find out if the various PearlBee::Dashboard:: modules had an import method or not. It looks like the use Dancer2 appname => 'PearlBee'; line provides them with one.

I wonder if Dancer2 does that on purpose, or if that method "leaked" somehow.

book commented 8 years ago

After looking at the Dancer2 source code, an actual import method is indeed created by Dancer2 in the corresponding namespaces. The code of that import is visible here and is equivalent to:

sub import {
    my ( $self, %options ) = @_;

    my $with = $options{with};
    for my $key ( keys %$with ) {
        $self->dancer_app->setting( $key => $with->{$key} );
    }
}

That code only does something if there's a list of parameters given, so in our case (no arguments to the use calls) there are no consequences.

andrewalker commented 8 years ago

Great! I agree that it is an acceptable compromise, and that the global is much better than post.url_for( settings ). And good that there are no consequences in replacing the uses for requires. So... let's merge it? @xsawyerx?

xsawyerx commented 8 years ago

@book and I decided on the next actions in order to merge this feature in. I'll work on it further and merge it.

andrewalker commented 8 years ago

Cool, @xsawyerx and @book :smile:

xsawyerx commented 8 years ago

Per conversations with @book and @andrewalker I have taken the following actions:

The commits in this branch had been merged, so the PR is effectively merged. Closing this ticket. :+1: !