Refactored generic_page_renderer and generic_post_list_renderer. #2519

felixfontein · 2016-10-02T19:55:16Z

Preliminary work for @Kwpolska's suggestion in getnikola/plugins#175.

…e common function generic_renderer for some of the dirty work.

Kwpolska

Looks good, but could use some more documentation, and a small bug fix.

Kwpolska · 2016-10-08T11:54:00Z

nikola/nikola.py

@@ -1986,38 +1986,27 @@ def scan_posts(self, really=False, ignore_quit=False, quiet=False):
            sys.exit(1)
        signal('scanned').send(self)

-    def generic_page_renderer(self, lang, post, filters, context=None):
-        """Render post fragments to final HTML pages."""
+    def generic_renderer(self, lang, output_name, template_name, filters, deps=[], uptodate_deps=[], pre_context=None, post_context=None, context_deps_remove=None, post_deps_dict=None, url_type=None):


You can’t use deps=[], uptodate_deps=[]. You should also document what arguments mean.

I renamed deps to file_deps. I also now documented all arguments.

Kwpolska · 2016-10-08T11:54:32Z

nikola/nikola.py

-            deps_dict['PREV_LINK'] = [post.prev_post.permalink(lang)]
-        if post.next_post:
-            deps_dict['NEXT_LINK'] = [post.next_post.permalink(lang)]
+        if context_deps_remove:


Could use a comment to explain this.

Do you mean what context_deps_remove does? That should now be covered in the argument documentation. Please tell me if that's enough.

It’s enough.

Kwpolska · 2016-10-09T09:34:12Z

nikola/nikola.py

@@ -1986,38 +1986,40 @@ def scan_posts(self, really=False, ignore_quit=False, quiet=False):
            sys.exit(1)
        signal('scanned').send(self)

-    def generic_page_renderer(self, lang, post, filters, context=None):
-        """Render post fragments to final HTML pages."""
+    def generic_renderer(self, lang, output_name, template_name, filters, file_deps=[], uptodate_deps=[], pre_context=None, post_context=None, context_deps_remove=None, post_deps_dict=None, url_type=None):


I meant, you can’t provide an empty list as a default argument to file_deps and uptodate_deps.

Ah, I forgot about that trap... I'll fix it.

Kwpolska

Will merge when tests pass.

felixfontein · 2016-10-09T09:44:39Z

One note: this will cause all pages rendered with generic_post_list_renderer to be rendered again as their deps_dict now also contains OUTPUT_FOLDER and TRANSLATIONS. (It didn't have that before.) Should I move them so they're only added for generic_page_renderer, or is it ok if generic_post_list_renderer also adds them? (For me, the latter seems more natural and that's why I implemented it this way.)

felixfontein · 2016-10-09T09:45:55Z

Also, another thing: the difference between pre_context and post_context is now only that only post_context allows to override lang, while lang in pre_context will be overridden. Assuming that you never want to override lang, how about merging them to one variable?

Kwpolska · 2016-10-09T09:47:54Z

Wouldn’t that dependency come from the GLOBAL_CONTEXT anyway? Or do you mean something else?
👍 for that merge, overriding lang sounds like a very stupid thing to do — let’s not support it.

felixfontein · 2016-10-09T09:50:34Z

So far GLOBAL_CONTEXT was merged in by the callers of generic_post_list_renderer and generic_page_renderer, but of course not everyone has to do that.
Ok, I'll fix that then.

Kwpolska · 2016-10-09T09:55:22Z

nikola/nikola.py

        file_deps += self.template_system.template_deps(template_name)
        file_deps = sorted(list(filter(None, file_deps)))

-        context = copy(pre_context) if pre_context else {}
+        context = copy(context) if context else {}


Do we need a second copy here, just to override lang?

Yes, in case someone calls this function with the same context for different languages in a row. That person will be quite surprised if they all end up with the same lang value in their context.

Got it. Merging

felixfontein · 2016-10-09T10:03:25Z

Thanks for merging. I'll now update the plugin accordingly.

Refactored generic_page_renderer and generic_post_list_renderer to us…

7d0f2e4

…e common function generic_renderer for some of the dirty work.

felixfontein mentioned this pull request Oct 2, 2016

Added simple "create 404 page" plugin. getnikola/plugins#175

Merged

felixfontein added 2 commits October 2, 2016 22:04

Making flake8 happy.

da50f07

Added url_type argument to generic_renderer as well.

6e35a77

Kwpolska requested changes Oct 8, 2016

View reviewed changes

felixfontein added 2 commits October 9, 2016 11:24

Renaming deps to file_deps.

16d3095

Documented arguments.

75054f3

Kwpolska reviewed Oct 9, 2016

View reviewed changes

Working around a Python trap.

f36c640

Kwpolska approved these changes Oct 9, 2016

View reviewed changes

Just having one context argument for generic_renderer.

175ca48

Kwpolska reviewed Oct 9, 2016

View reviewed changes

Kwpolska merged commit 30ec729 into master Oct 9, 2016

Kwpolska deleted the generalize-generic-renderers branch October 9, 2016 10:02

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Refactored generic_page_renderer and generic_post_list_renderer. #2519

Refactored generic_page_renderer and generic_post_list_renderer. #2519

felixfontein commented Oct 2, 2016

Kwpolska left a comment

Kwpolska Oct 8, 2016 •

edited

felixfontein Oct 9, 2016

Kwpolska Oct 8, 2016

felixfontein Oct 9, 2016

Kwpolska Oct 9, 2016

Kwpolska Oct 9, 2016

felixfontein Oct 9, 2016

felixfontein Oct 9, 2016

Kwpolska left a comment

felixfontein commented Oct 9, 2016

felixfontein commented Oct 9, 2016

Kwpolska commented Oct 9, 2016

felixfontein commented Oct 9, 2016

Kwpolska Oct 9, 2016

felixfontein Oct 9, 2016

Kwpolska Oct 9, 2016

felixfontein commented Oct 9, 2016

Refactored generic_page_renderer and generic_post_list_renderer. #2519

Refactored generic_page_renderer and generic_post_list_renderer. #2519

Conversation

felixfontein commented Oct 2, 2016

Kwpolska left a comment

Choose a reason for hiding this comment

Kwpolska Oct 8, 2016 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Kwpolska left a comment

Choose a reason for hiding this comment

felixfontein commented Oct 9, 2016

felixfontein commented Oct 9, 2016

Kwpolska commented Oct 9, 2016

felixfontein commented Oct 9, 2016

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

felixfontein commented Oct 9, 2016

Kwpolska Oct 8, 2016 •

edited