-
Notifications
You must be signed in to change notification settings - Fork 82
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
Added Metadata Plugin and documentation. #93
Conversation
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 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
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. |
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
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 |
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. |
As a side note, about the one-legged CLtL2 agrees:
I think I will make |
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
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.
That is not an assumption I've seen when reading CL code in the wild, although when/unless is common when writing imperative code.
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. |
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. |
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?
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.
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
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. |
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
Interesting idea, we could have the document class behave like a map, with all the extra keyword arguments being available using a setfable place |
Voila! I created a complete content extension system in PR #96 and revised So for now, lets forget this specific PR and see if my new content system proposal works as you would hope. |
I forgot to answer the following:
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. |
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 bothsummary card
andsummary card with large image
types but you might want to keep it around in case it is already in people's configurations.