Added Metadata Plugin and documentation.

[kenanb]

The Metadata Plugin adds SEO metadata, Open Graph metadata (the metadata most social platforms including Facebook use) and Twitter Cards specific metadata. The documentation is provided. So far I tested it with many different combinations of header data and keyword argument combinations. They all worked as intended.

Some notes on implementation:

The plugin activation needs to keyword arguments:
:twitter keyword argument sets "site" property of Twitter. (That is the Twitter account the Card is going to be bound to.)
Twitter Card specific metadata will be omitted if this property is not set because this option is mandatory.
:card keyword argument sets the type of Twitter card created.
Possible types are :summary and :image. Defaults to :summary.

The og:url property doesn't currently seem essential in coleslaw because a post has only one address. og:url (or twitter:url which defaults to og:url actually) is important when there are multiple url's to same post. But I added it as well just to make sure it is there when it becomes important.
I've seen your comment about twitter:url on twitter-summary-cards plugin which made me think if there is a pitfall to the seemingly obvious approach to getting the url. I would be happy to check my method and inform me if I am doing something wrong here: (concatenate 'string (domain *config*) "/" (namestring (page-url post)))

As with the twitter:url, some other properties like twitter:description are intentionally omitted, because those default to Open Graph tags that are already provided, which prevents duplication.

The description for Open Graph metadata is created from post content if not explicitly stated in post header because some platforms make it mandatory. But it is omitted from SEO description when not stated by user. Because Google is does a better job of crawling description strings from content than me. :) And also it is better SEO practice to not provide a description tag at all than coming up with a possible bad one. The content is stripped of markup tags and quotation marks first before added to metadata because Facebook doesn't hide tags when they explicitly exist in the description and quotations in the description break HTML file.

The SEO keywords are created from tags when they are not explicitly stated in post header.

The image to be used in sharing is either and absolute URL or a root-relative URL like /static/image.png

This plugin also makes the twitter-summary-card plugin unnecessary because it handles both summary card and summary card with large image types but you might want to keep it around in case it is already in people's configurations.

[kenanb]

As can be seen from this plugin, plugins need to be able to add extra keywords to content classes. My method works in this context but things will get complicated when the number of content types increase and same plugin adds functionality to multiple content types.

A possibly more modular approach would be redefining/subclassing content classes that needs to be manipulated by the plugin from inside the plugin. But then if a user creates posts while using the plugin and then disable it, the parser is going to error for Invalid initialization argument for keywords that exist in post headers but absent from class description.

In that case we need to make parser more forgiving against post headers, which will cause coleslaw to skip typo errors in headers as well, which is bad.

So my best bet is to create a coleslaw:*valid-header-tags* variable that holds the list of all possible keywords that are used for all content types. Example:

(defvar *valid-header-tags* 
             '((content . (:url :date :tags :text)) 
               (post . (:title :author :format :keywords :description :image :card :creator) 
               (page . (:title :author) 
               (shouts . (:title :author :format :link :image)))))

Every new plugin can register their respective header tags to this variable. The error checking for post headers can be done explicitly using this variable, first checking possible tags for content superclass, then the respective subclass.

This approach would possibly make it eaiser to add/delete/modify plugins in repository.

[PuercoPop]

Hi thanks for the pull request. From my perspective, this is a feature that I would like to see in Coleslaw.

The main issue with the PR is that it modifies the post class instead of extending it, which there currently is no mechanism for.

One way to to allow the plugins to register 3rd party content classes would be to modifying discover. Currently discover takes the extension of the file as the name of the corresponding content class. We could add an extra layer of indirection by having a mapping between extensions. This would conflict with the current implementation of discover. So I propose the following changes:

1. Add an export a variable named file-extension-content-class. It would be an alist that maps file-extensions to classes.
2. The default discover method would iterate for all the extensions map to the doc-type.
3. Make construct a generic-function and export it, to have it as an extension point(?)
4. Maybe do the same for add-document (?).

If @kingcons amenable to the PR I'm willing the implement the aforementioned changes and update the PR to use it..

On a more general note consider adding docstrings to functions and variables. I'll comment on other specific details in the file as well.

my 2c and Thank you again for your interest in Coleslaw

[kenanb]

Thank you very much for the warm welcome! :) My interest is not new tho, I've been using coleslaw for a years now. :)) Great piece of software really.

And thank you for the comments! Especially for the line-by-line review. I'll revise the PR accordingly.

Even though I am a huge fan of literate programming practices (as can be seen here) I feel like it is a little overkill adding docstrings to functions defined in this commit. Their functionality is obvious from the function names and none of them are exported. The docstrings will look like function names. But generally, I actually love docstrings. :)

About the modifying class problem, that was exactly my point in my second comment to the PR. But as you mentioned there is no extension mechanism right now, and even if there was, file-extension content-class mapping mechanism is not the only change that is needed for making it work. Parser should also be modified to accept all possible keywords that are provided in content extension plugins instead of only the ones that are enabled. I am 100% with you on the need to have a formal way of extending content classes (again, my second comment) but I think doing this properly is going to take some more planning.

Meanwhile, the PR doesn't break anything, the functionality is complete, and the protocol is pretty solid. So it is not going to break anything if we isolate it from the post class when the content class extension mechanism is added. The PR modifies the class directly only by extending it. It doesn't touch any of the already available functionality. And all the slots that are added by the PR are completely optional. They break neither past content created by users nor risk breaking compability in the future.

So I would say we merge it, and we can simply cut paste the added slots to extension mechanism in the plugin file when we add the extension mechanism.

[kenanb]

As a side note, about the one-legged if: I can change it if that is the convention of the repository but I generally consider ifto be much more suitable (even when it is one-legged and nil return is not explicitly stated) than when in situations where return value (not side-effects) is essential to the expression. A when macro implies side-effects, it is specifically created to support eval of multiple statements when the condition is true. Even the name of the macro doesn't seem fitting inside a setter.

CLtL2 agrees:

As a matter of style, when is normally used to conditionally produce some side effects, and the
value of the when form is normally not used. If the value is relevant, then it may be stylistically
more appropriate to use and or if.

I think I will make if return nil for false explicitly instead of switching to when.

[PuercoPop]

Btw, I am only a collaborator, so I don't want my comments to come off as unnecessarily antagonistic. The final decision is in hands of @kingcons

I feel like it is a little overkill adding docstrings to functions

This goes against my experience. When I'm other peoples code, the absence of docstrings complicates the initial understanding. Sure I can use slime-xref functionality to see where the variable or function is used and understand its purpose but the docstring can save time an facilitate the understanding of the codebase. As Coleslaw strives for being easy to hack, docstring coverage should aim for 100% imho.

I forgot to add that your valid header tags suggestion is spot on. I'll try to implement it asap.

Regarding the if, I prefer to use when/unless for one branch statements as the reader does not have to read the forms to know if there is an else form.

A when macro implies side-effects,

That is not an assumption I've seen when reading CL code in the wild, although when/unless is common when writing imperative code.

So I would say we merge it, and we can simply cut paste the added slots to extension mechanism in the plugin file when we add the extension mechanism.

I'll defer that decision to @kingcons, My goal was to give you prompt feedback so that you don't feel that your contribution is not ignored.

[kenanb]

I felt no antagonism at all. Sorry if I sound offended. I wasn't. I think it is great that you are pushing for perfect commits. I was just stating the reasoning behind my decisions for the code. Actually you made me change my mind. I am writing a content extension system right now. It is shaping up pretty nicely IMHO.

So I suggest delaying this PR just a little bit more so I can see if the extension system works as expected. If it does, I will make a PR out of it so we can discuss if it makes sense. If it gets accepted I will also modify this branch for the extension system. If it doesn't then we can talk about if we should wait for the extension system or not.

[kingcons]

Hey all, sorry I'm late to this thread.

@kenanb This is valuable work and thank you for writing good documentation and a well-explained PR! 🎉 🍰 🍸

@PuercoPop Thanks for giving this a first look over and providing Kenan with good feedback.

I'll ask one question up front and then follow up on the three major points of contention I've noticed. (If there are others I've missed, please say so!)

My question: Should this supersede/replace the existing twitter-summary-cards plugin or should they coexist? I assume this is a complete superset of the functionality, is that right?

As Coleslaw strives for being easy to hack, docstring coverage should aim for 100% imho.

I love this goal but I'm willing to relax it a bit in the case of plugins. I do think it's worth every function in the core src having a docstring, no matter how "self-explanatory" the code may be.

A when macro implies side-effects,

I'm actually surprised to see this and had either missed or forgotten this distinction between when and a one-legged if. I will say I strongly prefer when where possible. In light of this new information though, if you must use one-legged if I would omit the explicit nil/false branch or (even better) restructure the code to not depend on the return value of the if perhaps. I think I'm just biased against one-legged ifs. :)

The main issue with the PR is that it modifies the post class instead of extending it.

I happen to agree with this point. Doubling the size of the built-in slots on the post class to accommodate an optional plugin is something I'm not comfortable with. That being said, we do need a real mechanism for plugins that require arbitrary data to be kept with content objects.

Perhaps we should have all documents have a "extra-args" slot that contains any initargs which go unused? That sounds tricky but I bet if I dug into my copy of Keene it would be doable without too much nastiness. I'm open to suggestions on this point.

[PuercoPop]

This code is the essence of what I have in mind to allow the user to extend the document type

I still have to add the extended headers processing. Will do that tomorrow hopefully. We could provide a generic function {read,parse}-content with a different implementation for each document type which would allow maximum flexibility but I think it would be overkill and tedious for the user doing the extension. Using @kenanb's idea and hiding it behind an :additional-headers keyword argument in register-content-class would probably be the better idea.

Perhaps we should have all documents have a "extra-args" slot that contains any initargs which go unused? That sounds tricky but I bet if I dug into my copy of Keene it would be doable without too much nastiness. I'm open to suggestions on this point.

Interesting idea, we could have the document class behave like a map, with all the extra keyword arguments being available using a setfable place get-field or something similar.

[kenanb]

Voila! I created a complete content extension system in PR #96 and revised metadata plugin as an example to that. I also revised post content type to fit in the new system. The system is fully backwards compatible with the past user content. And I must say I am liking it. I hope you like it too. @kingcons @PuercoPop

So for now, lets forget this specific PR and see if my new content system proposal works as you would hope.

[PuercoPop]

I forgot to answer the following:

My question: Should this supersede/replace the existing twitter-summary-cards plugin or should they coexist? I assume this is a complete superset of the functionality, is that right?

This plugin subsumes the functionality of the twitter-summary-card plugin. Maybe we could add a warning pointing to the metadata plugin when loading the twitter-summary plugin.