Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Extension support #29

Open
wants to merge 12 commits into
base: master
from

Conversation

@doddo
Copy link
Contributor

commented Nov 10, 2018

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.

@doddo doddo changed the title Instaplerd support Extension support Nov 10, 2018

@jmacdotorg

This comment has been minimized.

Copy link
Owner

commented Nov 10, 2018

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

This comment has been minimized.

Copy link
Contributor Author

commented Nov 10, 2018

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

This comment has been minimized.

Copy link
Contributor Author

commented Nov 20, 2018

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: doddo/tuvix-insta@8f67769

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.

@doddo doddo force-pushed the doddo:instaplerd_support branch from e1fdd9d to 220aefe Dec 16, 2018

@jmacdotorg
Copy link
Owner

left a comment

This is very interesting work!

Beyond a couple of small POD edits, I would like to see some more clarity in how an extension might actually work? The test extension that you include would be a good opportunity for this. If I read it correctly, it's currently a no-op. I'd prefer if it creates an output file of some kind.

lib/Plerd.pm Outdated Show resolved Hide resolved
lib/Plerd.pm Outdated Show resolved Hide resolved
@@ -0,0 +1,13 @@
package TestExtension;

This comment has been minimized.

Copy link
@jmacdotorg

jmacdotorg Dec 23, 2018

Owner

Could you please expand this TextExtension module so that it actually writes a file into the docroot, based on the source file that it reads? That will allow the tests to pass without needing to modify the "+4" magic number (which is has changed to "+5" in more recent master-branch work, anyway) and would also serve to demonstrate a little more clearly how an extension might actually work in practice.

This comment has been minimized.

Copy link
@doddo

doddo Dec 24, 2018

Author Contributor

Aha! It does actually publish a file to the docroot, but it doesn't do that until the test extension is explicitly loaded into plerd, thats's at line 344, and you'll find the assert further down that it actually does publish a file, even though the source file was .dm.

But you are right that it's sort of a no-op. Let me see if I can make it do something more interesting.

t/basic.t Outdated Show resolved Hide resolved
@jmacdotorg

This comment has been minimized.

Copy link
Owner

commented Dec 23, 2018

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

@doddo doddo force-pushed the doddo:instaplerd_support branch from 220aefe to 5bc6ba2 Dec 24, 2018

doddo added some commits Dec 24, 2018

Make TestExtension do something
Now it does reverse the source file body...

Plus some docs are added too
@doddo

This comment has been minimized.

Copy link
Contributor Author

commented Dec 24, 2018

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

This comment has been minimized.

Copy link
Owner

commented Dec 24, 2018

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

This comment has been minimized.

Copy link
Contributor Author

commented Dec 24, 2018

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

This comment has been minimized.

Copy link
Contributor Author

commented Dec 25, 2018

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

This comment has been minimized.

Copy link
Owner

commented May 19, 2019

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:

  • 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.)

  • Plerd::Post becomes an abstract class, offering all the attributes and methods common to all post types. (Some of those methods will be stubs that die when invoked, complaining that the Post subclass at-hand failed to override them.)

  • This new abstract class has a new attribute: a list of file extensions that it's interested in handling. It also has a new class method: registering itself as those extensions' handler with its Plerd object (the one it already carries via $self->plerd).

  • Add a subclass of Plerd::Post called Plerd::Post::Markdown, and move all of the old Plerd::Post's Markdown-specific logic into it. (This gets shipped with Plerd, henceforth.)

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

This comment has been minimized.

Copy link
Contributor Author

commented May 19, 2019

* 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

This comment has been minimized.

Copy link
Owner

commented May 19, 2019

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

This comment has been minimized.

Copy link
Contributor Author

commented May 19, 2019

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.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
2 participants
You can’t perform that action at this time.