lektor / lektor-website

The main lektor website.
https://www.getlektor.com/
Other
160 stars 134 forks source link

New Plugin: read-full-post #181

Closed Andrew-Shay closed 6 years ago

Andrew-Shay commented 6 years ago

This is my first plugin. read-full-post
It allows the blog listing page to show a shorter version of the blog post.

However, I need some help.
The plugin worked in a sample lektor site when installed manually, however when installed via lektor, it doesn't appear to work.
I used the lektor command to install it.
I see

Installing collected packages: lektor-read-full-post
Successfully installed lektor-read-full-post-0.1

I also see lektor-read-full-post = 0.1 (added automatically) in my project config.
I made the changes to the templates but I don't see the plugin's changes.
Can someone help me debug? My develop branch contains the readme and code https://github.com/Andrew-Shay/lektor-read-full-post/tree/develop
It is on pypi https://pypi.python.org/pypi?name=lektor-read-full-post&version=0.1&:action=display

nixjdm commented 6 years ago

I just tried this plugin with lektor plugins add lektor-read-full-post, and it works for me. Good plugin!

Have you tried either lektor clean or lektor plugins flush-cache? It sounds like you have something hanging around when it shouldn't. If those commands don't clear things out sufficiently, you can see where your build and packages caches are with lektor project-info and lektor plugins list respectively.

nixjdm commented 6 years ago

A suggestion about the plugin though: You chose a good default string as a string to split on, but not everyone uses Markdown for their content fields. They might be using html straight, or rst with another plugin for instance (and others potentially). Your plugin would be more flexible if you could set the split text string (or list of strings to check for / split on) in the config, so people could supply their own. You just need to instruct people that they should be comments in their content type of choice so that they remain invisible on the fully rendered page.

Edit: Or, you could split the post and take the 0th element for the post snippet, and in the full post case, remove the 'split string' so it actually isn't even present anymore. It doesn't even need to be a comment in that case.

Andrew-Shay commented 6 years ago

Thanks for your feedback. It was a bug in my own plugin not reading the config the way I expected haha

I like both your suggestions! I will implement them :100:

Andrew-Shay commented 6 years ago

@nixjdm You mentioned people may not use Markdown. Do you know if it's possible to have posts with multiple formats on the same site?
I ask because I've been playing with different options to maximize support.
I was thinking about the following. This allows the user to create the code for the link and the split text. They are both csvs to allow multiple options. The split text detected would then pull the corresponding link-text at the same index.

link-text = <br><br>[Read Full Post]({URL_PATH}),<br><br><a href="{URL_PATH}">Continue Reading</a>
always-display = true
split-text = [//]: # (PLUGIN-READ-FULL-POST),<!-- PLUGIN-READ-FULL-POST -->

The only issue is if always-display is True, and there is no split text, which do I pick? Maybe the values at index 0? Maybe users should use HTML as the default? Would that provide max support?

nixjdm commented 6 years ago

Do you know if it's possible to have posts with multiple formats on the same site?

Yes, this is field-specific, specified by the content type, so a site / post could have multiple fields of different content types. Though I don't imagine that's very common to have back-to-back Markdown and pure HTML, but it'll happen on occasion.

If users know the first value of the link-text is default, I'd imagine that's usable enough for most people.

You could get fancier, if you wanted. For instance, you can check the type(post._data['body']) directly, and let users assign link-text to a dict instead of a list, so you can make the determination on the fly. That way you don't have one default, but have a mapping to each content type. They'll be able to explicitly say 'this is the markdown link', and 'this is the restructuredtext link'. That's more powerful. Whether it's easier to use is arguable. It's also more work :) It's up to you, but those are my thoughts.

nixjdm commented 6 years ago

This definitely falls under "premature generalization," but I noticed something else. You're assuming the user has named their field body and wants to operate on that, which might not be the case. Maybe they named it blogpost or anything else. If the user supplied blogpost and got back body_short, that'd be inconsistent too.

body_short might also conflict with an existing body_short the user may have made, too, which is a potential edge case where things would go awry.

I don't think you need to worry about either of these soon, if ever, but you got my noddle cooking, so I thought I'd write these down while I'm thinking about it all.

Andrew-Shay commented 6 years ago

I like your new advice again! For type(post._data['body']) I get <class 'lektor.types.formats.MarkdownDescriptor'> I could use type(post._data['body']).__name__ to detect what it is. However this isn't listed on the Builtin Field Types page

Andrew-Shay commented 6 years ago

I did find post.datamodel.field_map['body'].type.name which returns markdown EDIT: Think it'll be okay to use this?

Andrew-Shay commented 6 years ago

@nixjdm

nixjdm commented 6 years ago

Hi @Andrew-Shay, I like the last way you found better post.datamodel.field_map['body'].type.name since that gives the name in the exact same way the user would have to list the type of the body field in the ini model file. That seems easiest for the user. Plus, it doesn't access any private methods which looks better code-wise. :)

Note, this should work even for types other plugins add, such as asciidoc or rst.

Andrew-Shay commented 6 years ago

@nixjdm Will you test my plugin again? :smile:
I'm hoping it's finished

nixjdm commented 6 years ago

@Andrew-Shay Sure, I can get check it out tomorrow. I was wondering how it's been coming along. Also, can you rebase this PR? There's a conflict now, for some reason. cough I have no idea why. cough

Andrew-Shay commented 6 years ago

Ill be sure to rebase before requesting the merge! :)

nixjdm commented 6 years ago

@Andrew-Shay It doesn't look like you've pushed any commits since we've discussed all those options / changes https://github.com/Andrew-Shay/lektor-read-full-post/tree/develop

Do you just need to push?

Andrew-Shay commented 6 years ago

@nixjdm Derp. Pushed.

nixjdm commented 6 years ago

Hey @Andrew-Shay,

You should note the default for always-display option in the README. When it's not included it has the same effect of false, which I like.

Only markdown is really handled, basically, because most Lektor content types that are string-like don't have a .source where their contents are stored internally.

File "/home/joe/wrkspc/lktr-prjcts/lektor/example/packages/lektor-read-full-post/lektor_read_full_post.py", line 36, in rfp_process_post
    text_full = post._data['body'].source
AttributeError: 'Markup' object has no attribute 'source'

To handle html as well, and most everything else, you'll need to do something similar to what I did for the jinja-content plugin.

That's all I've caught so far!

Once conflicts are resolved, I can pull this whenever you're satisfied, by the way. Just let me know. I'm happy to keep doing code review if you like, but you may end up waiting on me some :)