-
Notifications
You must be signed in to change notification settings - Fork 311
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
Page context plugin #141
base: master
Are you sure you want to change the base?
Page context plugin #141
Changes from 3 commits
e960c2a
36b228a
6e2e3d0
7cb0068
ed84669
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -75,16 +75,11 @@ def context(self, data=None, extra=None): | |
""" | ||
The page context. | ||
""" | ||
if extra is None: | ||
extra = {} | ||
|
||
context = {'__CACTUS_CURRENT_PAGE__': self,} | ||
|
||
page_context, data = self.parse_context(data or self.data()) | ||
|
||
context.update(self.site.context()) | ||
context.update(extra) | ||
context.update(page_context) | ||
if extra: | ||
context.update(extra) | ||
|
||
return Context(context) | ||
|
||
|
@@ -93,15 +88,8 @@ def render(self): | |
Takes the template data with context and renders it to the final output file. | ||
""" | ||
|
||
data = self.data() | ||
context = self.context(data=data) | ||
|
||
# This is not very nice, but we already used the header context in the | ||
# page context, so we don't need it anymore. | ||
page_context, data = self.parse_context(data) | ||
|
||
context, data = self.site.plugin_manager.preBuildPage( | ||
self.site, self, context, data) | ||
self.site, self, self.context(data=self.data()), self.data()) | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. See note above — we might as well remove |
||
|
||
return Template(data).render(context) | ||
|
||
|
@@ -125,37 +113,5 @@ def build(self): | |
|
||
self.site.plugin_manager.postBuildPage(self) | ||
|
||
def parse_context(self, data, splitChar=':'): | ||
""" | ||
Values like | ||
|
||
name: koen | ||
age: 29 | ||
|
||
will be converted in a dict: {'name': 'koen', 'age': '29'} | ||
""" | ||
|
||
if not self.is_html(): | ||
return {}, data | ||
|
||
values = {} | ||
lines = data.splitlines() | ||
if not lines: | ||
return {}, '' | ||
|
||
for i, line in enumerate(lines): | ||
|
||
if not line: | ||
continue | ||
|
||
elif splitChar in line: | ||
line = line.split(splitChar) | ||
values[line[0].strip()] = (splitChar.join(line[1:])).strip() | ||
|
||
else: | ||
break | ||
|
||
return values, '\n'.join(lines[i:]) | ||
|
||
def __repr__(self): | ||
return '<Page: {0}>'.format(self.source_path) |
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,120 @@ | ||
#coding:utf-8 | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Can you name this file |
||
import logging | ||
import re | ||
|
||
|
||
logger = logging.getLogger(__name__) | ||
|
||
|
||
class PageContextPlugin(object): | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. The "contract" used to be that context would be loaded before any plugin runs. Can you ensure that this continues to be the case here? Plugins are sorted as per their This will need tests as well. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. But this "contract" would mean that context handling would not be pluggable. Certainly you are right, this plugin has to have a low |
||
""" | ||
A plugin to extract context variables from the top of the file | ||
|
||
TODO: | ||
- read style of metadata from config | ||
- read splitChar from config | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. What's the plan for those? Note: I'm not 100% sure the second option is that desirable. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. In the current implementation |
||
""" | ||
|
||
def preBuildPage(self, page, context, data): | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Do you test this entrypoint anywhere? For all we know this method could be completely broken (e.g. it might fail on empty files) and we wouldn't know without test coverage on it : ( In fact, I'd rather you test everything through this entrypoint, rather than call the individual methods below (ultimately, Cactus will call This would make it much easier to be confident our tests address edge cases (e.g. empty files, empty context, a file that's just a new line, etc.). Currently, we can't reliably tell whether an empty file or a file that just contains a newline will work or not, because some edge cases are handled in |
||
""" | ||
Update the page context with the context variables from the file | ||
and delete them | ||
""" | ||
if not page.is_html() or not data: | ||
return context, data | ||
|
||
lines = data.splitlines() | ||
|
||
# Look into the file to decide on the style of metadata | ||
# TODO: make that configurable | ||
if lines[0].strip() == u'---': | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. This might crash on empty files. Can we add a test for it? On second thought, it probably doesn't though, since you test for "not data" earlier. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. As a side note, adding the following to the test cases:
Results in:
(Note: I realize some of that broken code was actually present before) The problem here is that this might be OK considering you're resting for data not being empty is There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. The data can not be empty as of the current state of the code. Should I still test for it ( |
||
page_context, lines = self.jekyll_style(lines) | ||
else: | ||
page_context, lines = self.simple_style(lines) | ||
context.update(page_context) | ||
|
||
return context, '\n'.join(lines) | ||
|
||
def jekyll_style(self, lines): | ||
if not lines or lines[0].strip() != u'---': # no metadata | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Can you import There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Sure. I wasn't aware of |
||
return {}, lines | ||
|
||
context = {} | ||
splitChar = ':' | ||
for i, line in enumerate(lines[1:]): | ||
if line.strip() == u'---': # end of metadata | ||
i += 1 | ||
break | ||
|
||
try: | ||
key, value = line.split(splitChar, 1) | ||
except ValueError: | ||
# splitChar not in line | ||
logger.warning('Page context data seem to end in line %d', i) | ||
break | ||
|
||
context[key.strip()] = value.strip() | ||
|
||
return context, lines[i+1:] | ||
|
||
def simple_style(self, lines): | ||
""" | ||
Only lines at the top of the file with a colon are metadata. | ||
No multiline metadata | ||
""" | ||
context = {} | ||
splitChar = ':' | ||
|
||
for i, line in enumerate(lines): | ||
|
||
if splitChar not in line: | ||
break | ||
|
||
key, value = line.split(splitChar, 1) | ||
context[key.strip()] = value.strip() | ||
|
||
return context, lines[i:] | ||
|
||
def multimarkdown_style(self, lines): | ||
""" | ||
metadata as defined in http://fletcher.github.io/MultiMarkdown-4/metadata | ||
""" | ||
context = {} | ||
splitChar = ':' | ||
fenced = False | ||
key = None | ||
|
||
if splitChar not in lines[0] and lines[0].strip() != u'---': | ||
return {}, lines # no metadata | ||
|
||
for i, line in enumerate(lines): | ||
# fencing is allowed but not required in multimarkdown | ||
if line.strip() == u'---': | ||
if i == 0: | ||
fenced = True | ||
continue | ||
elif fenced: | ||
i += 1 | ||
break | ||
|
||
# a blank line triggers the beginning of the rest of the document | ||
if not line.strip(): | ||
# if the file starts with a blank line, nothing should be changed | ||
if i != 0: | ||
i += 1 | ||
break | ||
|
||
# indented lines are the continuation of the previous value | ||
# but multiline values don't have to be indented | ||
if key and (re.match(r'\s', line) or not splitChar in line): | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. You're instantiating a regex in a loop. It would be preferable to compile it once, and then use the compiled regex in the loop. |
||
context[key] = ' '.join( | ||
(context[key], line.lstrip())) | ||
continue | ||
|
||
key, value = line.split(splitChar, 1) | ||
|
||
# keys are lower cased and stripped of all whitespace | ||
key = re.sub(r'\s', '', key.lower()) | ||
context[key] = value.strip() | ||
|
||
return context, lines[i:] |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think we should refactor this method.
data
is passed, but we never use it.extra
is never passed. How about we remove those altogether?Cheers,
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Sure, I had left it in because somebody else might rely on them being there. But that seems overly cautious.