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

Page context plugin #141

Open
wants to merge 5 commits into
base: master
Choose a base branch
from
Open
Show file tree
Hide file tree
Changes from 3 commits
Commits
File filter

Filter by extension

Filter by extension


Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
18 changes: 18 additions & 0 deletions README.md
Original file line number Diff line number Diff line change
Expand Up @@ -124,6 +124,24 @@ Cactus makes it easy to relatively link to pages and static assets inside your p

Just use the URL you would normally use: don't forget the leading slash.

You can add extra context to your pages. Just insert it on the top of the file like this:

name: koen
age: 29
{% extends "base.html" %}

which will be converted to a dict: `{'name': 'koen', 'age': '29'}` and added to the context.
If the first line of your document should contain a colon,
you can use the more explicit style of metadata [as used in Jekyll](http://jekyllrb.com/docs/frontmatter/):

---
Author: Me
Style: Great
---
Todays motto: whatever

Always use `---` (three dashes) to fence the metadata.

### Templates

Expand Down
50 changes: 3 additions & 47 deletions cactus/page.py
Original file line number Diff line number Diff line change
Expand Up @@ -75,16 +75,11 @@ def context(self, data=None, extra=None):
"""
Copy link
Collaborator

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,

Copy link
Contributor Author

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.

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)

Expand All @@ -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())
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

See note above — we might as well remove self.data() here, since we don't use it.


return Template(data).render(context)

Expand All @@ -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)
120 changes: 120 additions & 0 deletions cactus/plugin/builtin/pagecontext.py
Original file line number Diff line number Diff line change
@@ -0,0 +1,120 @@
#coding:utf-8
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Can you name this file page_context.py? This would be more consistent with e.g. template_tags.py and generally how Python files are called

import logging
import re


logger = logging.getLogger(__name__)


class PageContextPlugin(object):
Copy link
Collaborator

Choose a reason for hiding this comment

The 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 ORDER property. You can probably design an ORDER property that sorts lower than everything (and therefore always loads first).

This will need tests as well.

Copy link
Contributor Author

Choose a reason for hiding this comment

The 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 ORDER. Is there any documentation or agreement on the expected / desirable values for ORDER for different cases?

"""
A plugin to extract context variables from the top of the file

TODO:
- read style of metadata from config
- read splitChar from config
Copy link
Collaborator

Choose a reason for hiding this comment

The 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.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

In the current implementation splitChar is passed as a parameter, in this new version it is hard coded. Putting it into the config looks like a middle ground to me. But I'm also not sure, if it is necessary.

"""

def preBuildPage(self, page, context, data):
Copy link
Collaborator

Choose a reason for hiding this comment

The 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 preBuildPage, not the individual methods).

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 preBuildPage, and other edge cases are handled in the individual methods below.

"""
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'---':
Copy link
Collaborator

Choose a reason for hiding this comment

The 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.

Copy link
Collaborator

Choose a reason for hiding this comment

The 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:

======================================================================
ERROR: test_multimarkdown_style (cactus.tests.test_plugins.TestPageContextPlugin)
----------------------------------------------------------------------
Traceback (most recent call last):
  File "/Users/thomas/dev/cactus/cactus/tests/test_plugins.py", line 230, in test_multimarkdown_style
    plugin.multimarkdown_style(lines),
  File "/Users/thomas/dev/cactus/cactus/plugin/builtin/page_context.py", line 88, in multimarkdown_style
    if splitChar not in lines[0] and lines[0].strip() != u'---':
IndexError: list index out of range

======================================================================
ERROR: test_simple_style (cactus.tests.test_plugins.TestPageContextPlugin)
----------------------------------------------------------------------
Traceback (most recent call last):
  File "/Users/thomas/dev/cactus/cactus/tests/test_plugins.py", line 162, in test_simple_style
    plugin.simple_style(lines),
  File "/Users/thomas/dev/cactus/cactus/plugin/builtin/page_context.py", line 77, in simple_style
    return context, lines[i:]
UnboundLocalError: local variable 'i' referenced before assignment

(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 preBuildPage, but that we can't test it properly to make sure.

Copy link
Contributor Author

Choose a reason for hiding this comment

The 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 (if not lines: return {}, [])? I added a comment in the code to make it clear for future developers.

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

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Can you import unicode_literals rather than use u'---' everywhere?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Sure. I wasn't aware of unicode_literals.

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):
Copy link
Collaborator

Choose a reason for hiding this comment

The 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:]
8 changes: 6 additions & 2 deletions cactus/site.py
Original file line number Diff line number Diff line change
Expand Up @@ -16,6 +16,7 @@
from cactus.plugin.builtin.cache import CacheDurationPlugin
from cactus.plugin.builtin.context import ContextPlugin
from cactus.plugin.builtin.ignore import IgnorePatternsPlugin
from cactus.plugin.builtin.pagecontext import PageContextPlugin
from cactus.plugin.loader import CustomPluginsLoader, ObjectsPluginLoader
from cactus.plugin.manager import PluginManager
from cactus.static.external.manager import ExternalManager
Expand Down Expand Up @@ -75,8 +76,11 @@ def __init__(self, path, config_paths=None, ui=None,
[
CustomPluginsLoader(self.plugin_path), # User plugins
ObjectsPluginLoader([ # Builtin plugins
ContextPlugin(), CacheDurationPlugin(),
IgnorePatternsPlugin(), PageContextCompatibilityPlugin(),
ContextPlugin(),
PageContextPlugin(),
CacheDurationPlugin(),
IgnorePatternsPlugin(),
PageContextCompatibilityPlugin(),
])
]
)
Expand Down
64 changes: 52 additions & 12 deletions cactus/tests/test_context.py
Original file line number Diff line number Diff line change
Expand Up @@ -46,21 +46,61 @@ class TestCustomPageContext(SiteTestCase):

Includes the built-in site context ('CACTUS'), and custom context.
"""
def setUp(self):
super(TestCustomPageContext, self).setUp()

page = "a: 1\n\tb: Monkey\n{{ a }}\n{{ b }}"
with open(os.path.join(self.site.page_path, "test.html"), "w") as f:
f.write(page)

self.site.build()

def test_custom_context(self):
"""
Test that custom context is provided
"""
with open(os.path.join(self.site.build_path, "test.html")) as f:
a, b = f.read().split("\n")
test_data = [
("a: 1\n"
"\tb: Monkey\n"
"{{ a }}\n"
"{{ b }}",

self.assertEqual(a, "1")
self.assertEqual(b, "Monkey")
"1\n"
"Monkey"),

("a: 1\n"
"\n" # blank line inserted
"\tb: Monkey\n"
"{{ a }}",

"\n"
"\tb: Monkey\n"
"1"),

("---\n" # fenced
" a: 1\n"
"b: Monkey\n"
"---\n"
"{{ a }}\n"
"{{ b }}",

"1\n"
"Monkey"),

("---\n" # double fenced
" a: 1\n"
"b: Monkey\n"
"---\n"
"---\n"
"c: 1\n"
"---\n"
"{{ a }}\n"
"{{ b }}",

"---\n"
"c: 1\n"
"---\n"
"1\n"
"Monkey"),

]
for page, expected in test_data:
with open(os.path.join(self.site.page_path, "test.html"), "w") as f:
f.write(page)

self.site.build()

with open(os.path.join(self.site.build_path, "test.html")) as f:
self.assertEqual(f.read(), expected)
Loading