jmacdotorg / plerd

Ultralight Dropbox-friendly Markdown-based blogging.
MIT License
112 stars 21 forks source link

Extension support #29

Open doddo opened 6 years ago

doddo commented 6 years ago

This is my suggestion for adding extension support for Plerd. I use this to load Instaplerd which can parse jpegs and render images with various creative filters and such, (but its rather new so I have only made one filter so far). You can see it in production on my homepage, to get an idea of the thing.

I was trying to make this PR into something reusable, so that you can make blog posts from any source imaginable, as long as it's got an extension which implements the methods in Plerd::Post (there's a quite good example of this in the t folder), and has a "static" file_type method which can be used to deduce what files it likes.

jmacdotorg commented 6 years ago

Thank you very much for this work!

I am unable to examine this right away, but look forward to giving it a look as soon as I am able.

doddo commented 6 years ago

I am unable to examine this right away, but look forward to giving it a look as soon as I am able.

Good because I just added some finishing touches!!

doddo commented 6 years ago

I just added one final thing: extension_preferences as something where each individual plugin could be configured, i. e something along the line of:

extension_preferences:
    'InstaPlerd::Post':
        filter: 'InstaPlerd::Filters::Artistic'
        width: 800
        height: 800
        compression: 85

This way, it's up to each individual plugin how they want to treat the data in the plugin_settings block. This is how I did it in mine: https://github.com/doddo/instaplerd/commit/8f6776994bc0f90f1f22921edaa80eea83c531d6

I do think that that covers everything I need, and also: Having some basic extension support for plerd like this will allow it to remain lightweight while at the same time allowing people to go bananas.

jmacdotorg commented 5 years ago

Furthermore, I recommend merging in the most recent work on the master branch before performing any additional work here.

doddo commented 5 years ago

I think that I've addressed the points you raised, including having the TestExtension actually do something, albeit a rather pointless thing. I did also add some perldoc to help clarify things. Beyond doing something like this though, an extension would typically want to overwrite the _process_source_file, and since there is so much going on in that function I am not sure if it would clarify how extensions work or if it would just make it more confusing.

jmacdotorg commented 5 years ago

Thanks, this does bring me somewhat closer to enlightenment...

Is an extension always expected to be a Plerd::Post subclass, then? With the expectation that the $plerd instance will add its objects to its internal array of posts, and (from its point of view) treat them like traditional post objects?

doddo commented 5 years ago

Yes, and no it does not strictly have to be a subclass but it would have to implement all Plerd::Post methods which Plerd uses when dealing with posts, so even if subclassing strictly isn't necessary it sure makes things easier. And you are correct that in this current shape it does only extend the functionaly around handling posts, Other things such as Plerd itself could be extended too but I have no clear idea of what that would look like as I don't know what other functionality would benefit from extending it, although I suppose one such use case could be an extension which could copy assets after $plerd->post() is run that could be of value for some people.

doddo commented 5 years ago

I have in my head a concept which could work for extending Plerd using the moose before after and around hooks for all method and attributes which moose knows about. If you'd like i could enrich the pr with such functionality as well, that would make Plerd truly flexible!

jmacdotorg commented 5 years ago

Rather than thinking about this in terms of extensions with Markdown handling baked into the core, I'd rather see this refactored so that Plerd calls different file-handlers, according to an internal, user-defined registry -- and Markdown is just another of them (and happens to be the default).

Here's one way it could work:

After this, a Plerd file-handling extension would be implemented by subclassing Plerd::Post, overriding methods (and adding new ones) as necessary, and defining what file extensions it cares about. After getting loaded during running by way of a config file, it call its own class method in an attempt to register itself with the Plerd object. From that point on, Plerd will create an object of that class as a post, whenever it wishes to publish a source file of the kind it's interested in.

Make sense? What do you think?

doddo commented 5 years ago
* Plerd carries a registry attribute that maps file extensions to Plerd::Post subclasses. The default has two entries, mapping .md and .markdown to Plerd::Post::Markdown. (Which, see below.)

I made it like this: A post has some sort of attribute:

sub file_type {
    '(md|markdown)';
}

Then the Plerd has a "triggers" section which gets populated like this:

sub _build_post_triggers {
    my $self = shift;
    my %triggers;

    # "builtin" trigger is here
    $triggers{ Tuvix::ExtendedPlerd::Post->file_type } = 'Tuvix::ExtendedPlerd::Post';

    foreach my $classref (@{$self->extensions // []}) {
        if ($classref->can('file_type')) {
            $triggers{ $classref->file_type } = $classref;
        }
    }

    return \%triggers;
}

(Had the Plerd::Post been added as an extension then the explicit "builtin trigger" fix would not have made neccesary)

then finally, as seen in the PR (but slightly modified here) in the [https://github.com/doddo/tuvix/blob/master/lib/Tuvix/ExtendedPlerd.pm#L49](build posts) logic:

sub _build_posts {
    my $self = shift;
    my @posts;
    my $triggers = $self->post_triggers;

    foreach my $file (sort {$a->basename cmp $b->basename} $self->source_directory->children) {
        foreach my $trigger (keys %{$triggers}) {
            if ($file =~ m/\.$trigger$/i) {
                push @posts, $$triggers{$trigger}->new(plerd => $self, source_file => $file);
                last;
            }
        }
    }
    return [ sort {$b->date <=> $a->date} @posts ];
}

The rationale for this was that the various file_type attributes can then be made into a compound regular expression for the directory watcher:

 my $trigger = sprintf '\.(%s)$', join('|', keys %{$plerd->post_triggers});

Which in my real life schenario gets translated to this:

\.(jpe?g|(md|markdown))$

And this has been working solidly for my use case.

This could (arguably) be more convenient (ie having the plugins dictate themselves what files they want to deal with) than having an explicit registry, on the other hand then the control will be moved out of the config and one day maybe there will be overlapping plugins who both want to deal with the same file, but on the third hand, I see no such use case why you would want to load two plugins who deals with one particular file type.

All of this said, I think your proposal is solid and prudent. my jpeg addition is still written with eventual Plerd compatibility in mind so should be no problems to incorporate all of this. And it would be less of a hack with an abstract base class instead of overriding methods the way I been doing so far.

jmacdotorg commented 5 years ago

The rationale for this was that the various file_type attributes can then be made into a compound regular expression for the directory watcher

I've actually been wondering lately if this regex-based filter is still necessary. In Plerd, it's used only by the File::ChangeNotify instance to limit the kinds of files whose modification within the source triggers a publish_all call. But, in the years I've been using Plerd, I've found that I've never purposefully put a file in the source directory that I didn't Plerd to treat as a post.

I wonder about changing the File::ChangeNotify logic so that it ignores a Plerd-stored regex of filename patterns (covering e.g. dotfiles, tilde-files, and files without extensions), but treats everything else as a potential post, without worrying whether or not it's registered with Plerd. Similar to your current work, Plerd will then try to make a post of every non-ignorable file in the source directory; if its extension is in its registry, it hands it off to the correct Post subclass for instancing, and if it isn't, it skips it (and should probably log a warning).

I do prefer the idea of changing the registry into a nice, clean, attribute-stored hash like this, rather than a bag of regular-expression snippets. But would this approach be not flexible enough for the ways you've been using Plerd?

doddo commented 5 years ago

I do prefer the idea of changing the registry into a nice, clean, attribute-stored hash like this, rather than a bag of regular-expression snippets. But would this approach be not flexible enough for the ways you've been using Plerd?

Not in the least: plus that what you said about ignoring stuff which do not explicitly map to a suitable file. That is very true.