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

Refactored generic_page_renderer and generic_post_list_renderer. #2519

Merged
merged 7 commits into from Oct 9, 2016

Conversation

@felixfontein
Copy link
Contributor

felixfontein commented Oct 2, 2016

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

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

Kwpolska left a comment

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

@@ -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):

This comment has been minimized.

Copy link
@Kwpolska

Kwpolska Oct 8, 2016

Member

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

This comment has been minimized.

Copy link
@felixfontein

felixfontein Oct 9, 2016

Author Contributor

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

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:

This comment has been minimized.

Copy link
@Kwpolska

Kwpolska Oct 8, 2016

Member

Could use a comment to explain this.

This comment has been minimized.

Copy link
@felixfontein

felixfontein Oct 9, 2016

Author Contributor

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

This comment has been minimized.

Copy link
@Kwpolska

Kwpolska Oct 9, 2016

Member

It’s enough.

@@ -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):

This comment has been minimized.

Copy link
@Kwpolska

Kwpolska Oct 9, 2016

Member

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

This comment has been minimized.

Copy link
@felixfontein

felixfontein Oct 9, 2016

Author Contributor

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

This comment has been minimized.

Copy link
@felixfontein

felixfontein Oct 9, 2016

Author Contributor

Done.

Copy link
Member

Kwpolska left a comment

Will merge when tests pass.

@felixfontein
Copy link
Contributor Author

felixfontein commented Oct 9, 2016

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
Copy link
Contributor Author

felixfontein commented Oct 9, 2016

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
Copy link
Member

Kwpolska commented Oct 9, 2016

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

felixfontein commented Oct 9, 2016

  1. 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.
  2. Ok, I'll fix that then.
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 {}

This comment has been minimized.

Copy link
@Kwpolska

Kwpolska Oct 9, 2016

Member

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

This comment has been minimized.

Copy link
@felixfontein

felixfontein Oct 9, 2016

Author Contributor

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.

This comment has been minimized.

Copy link
@Kwpolska

Kwpolska Oct 9, 2016

Member

Got it. Merging

@Kwpolska Kwpolska merged commit 30ec729 into master Oct 9, 2016
3 of 4 checks passed
3 of 4 checks passed
continuous-integration/appveyor/pr Waiting for AppVeyor build to complete
Details
continuous-integration/appveyor/branch AppVeyor build succeeded
Details
continuous-integration/travis-ci/pr The Travis CI build passed
Details
continuous-integration/travis-ci/push The Travis CI build passed
Details
@Kwpolska Kwpolska deleted the generalize-generic-renderers branch Oct 9, 2016
@felixfontein
Copy link
Contributor Author

felixfontein commented Oct 9, 2016

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

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Linked issues

Successfully merging this pull request may close these issues.

None yet

2 participants
You can’t perform that action at this time.