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

Creating abstract base class BasePost. #2671

Closed
wants to merge 1 commit into from

Conversation

felixfontein
Copy link
Contributor

First shot at #2659 and #1709. Tried to move everything which is not input file dependent into base class.

@felixfontein
Copy link
Contributor Author

@Kwpolska: so what's bad about refactoring the Post class?

@Kwpolska
Copy link
Member

@felixfontein Overengineering.

@felixfontein
Copy link
Contributor Author

Do you really think that refactoring the Post class in general is overengineering? Or do you think it's a bad idea to support something like #2659?

@Kwpolska
Copy link
Member

I think that BasePost and the possibility to have different subclasses of Post is a bad idea, and a waste of time and resources. #2659 is a bit of a problematic thing. Nikola is tied to writing files to disk in many places, and you would need to mess with dependencies if you changed the meaning of source_path — and then there might be plugins which depend on that being a real file on disk, and would need a lot of special casing.

But perhaps the file-less posts could write to cache/, where a file has to exist anyway?

cc @remram44

@felixfontein
Copy link
Contributor Author

Aside from the point that some other plugins might assume that source_path is a real file on disk (which might be a problem, but which shouldn't be a reason not to do this), Nikola doesn't need the post files to exist on disk except for scan_post.py and the functions that I didn't move to BasePost. If you want a pure in-memory post (generated by whatever source), you can simply put the post's content into an uptodate dependency and emit that, and write it to disk in the overridden compile method. That would be a pretty simple implementation of another subclass of BasePost, and something rather impossible (without a lot of copy'n'pasting) with the current Post class from Nikola master; the main problem being all the logic done in __init__ which cannot be moved out now without causing incompatibility with existing plugins creating Post objects.

Creating artificial post files in cache/ is something I don't really like. Especially because then you have to make sure there aren't any collisions or other files which will be interpreted by Nikola as other-language versions of the same post, or as metadata. (If you have a post file containing metadata, and for some reason a file with the same name but .meta at the end is there containing metadata, I think Nikola will use that one and threat the metadata in the post file as content.) This would be a really messy solution.

@remram44
Copy link

If you think supporting file-less posts is not something that Nikola wants to support, I totally understand. You are building on top of doit, and most people use files. I respect wanting to keep the codebase simple.

I only requested #2659 because work on a PostScanner plugin had already been introduced in #1708, so I thought this was a direction you were considering -- after all, there is not much that can be done in terms of plugins if all you can do is readdir() a different directory.

I am in no way blocked by #2659 personally.

@Kwpolska
Copy link
Member

There is not much that can be done in terms of plugins if all you can do is readdir() a different directory.

You can, but a post scanner is usually not enough to do it. For example, our Plugins site uses pkgindex_scan and pkgindex_compiler — the scan plugin creates Post elements from README.md files found in the filesystem, but then the scanner adds meta data from .plugin files and handles compiling those README files. I don’t think there is a hard dependency on source_path being unique, so maybe this architecture (custom scanner + compiler) will be enough for you?

@felixfontein
Copy link
Contributor Author

Well, file-less posts are no problem even with doit, as long as you can efficiently create an uptodate object every run of Nikola which describes whether the source is still up-to-date. For an RSS feed, that should be no problem (assuming that the feed file isn't huge and parsing it takes a considerable amount of time).

The only way to implement something like this in Nikola is at the moment to completely rewriting the Post.__init__ method when subclassing the Post class, as its current implementation assumes there are files on disk.

Actually, there's one trick which would simplify the procedure without creating an abstract base class: move all the file-specific methods of the constructor into a method called by the constructor. Then subclasses of Post could override that method (and other methods, as they like to). This would still require to repeat some code (especially in the functions retrieving dependencies), but it would be much less bad than the current situation.

I'd still prefer an abstract base class, though. And I still disagree that this is overengineering; the Post class is one of the least modular/extensible parts of Nikola.

@gwax
Copy link
Contributor

gwax commented Jun 12, 2017

I think the principle of #1709 is sound but I'm not sure this is the right architectural direction. I'm on the fence about #2659.

It seems to me, if we're going to break up Post, we should start by pulling out some of the complex functionality first, rather than starting by abstracting everything.

Looking at Post.__init__ and some of the other code, it seems like creating a PostMetadata class and encapsulating the metadata might be a better solution than abstracting the metadata processing.

@felixfontein
Copy link
Contributor Author

I guess we should then close this and continue the discussion in #1709. (Also related is #2830.)

@Kwpolska Kwpolska deleted the adding-base-post-class branch September 20, 2018 18:34
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

4 participants