Skip to content
Closed
Show file tree
Hide file tree
Changes from all 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
75 changes: 40 additions & 35 deletions cactus/page.py
Original file line number Diff line number Diff line change
Expand Up @@ -42,6 +42,7 @@ def __init__(self, site, source_path):
else:
self.final_url = self.link_url
self.build_path = self.source_path
self._read_data()

def is_html(self):
return urlparse.urlparse(self.source_path).path.endswith('.html')
Expand All @@ -65,26 +66,27 @@ def full_build_path(self):
return os.path.join(self.site.build_path, self.build_path)

def data(self):
if not hasattr(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.

I'd rather you set _data to None (either in __init__ or in the class definition) and check for None. Functionally equivalent an plays much better with with refactoring tools.

self._read_data()
return self._data

def _read_data(self):
with open(self.full_source_path, 'rU') as f:
try:
return f.read().decode('utf-8')
self._data = f.read().decode('utf-8')
except:
logger.warning("Template engine could not process page: %s", self.path)
logger.warning("Page file could not be read: %s", self.path)
self._data = ''

def context(self, data=None, extra=None):
def context(self, 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)
context.update(extra or {})
context.update(self.parse_context())

return Context(context)

Expand All @@ -93,17 +95,10 @@ 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, self._data = self.site.plugin_manager.preBuildPage(
self.site, self, self.context(), self._data)

context, data = self.site.plugin_manager.preBuildPage(
self.site, self, context, data)

return Template(data).render(context)
return Template(self._data).render(context)
Copy link
Collaborator

Choose a reason for hiding this comment

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

Why access _data when you have the wrapper data() method?


def build(self):
"""
Expand All @@ -125,37 +120,47 @@ def build(self):

self.site.plugin_manager.postBuildPage(self)

def parse_context(self, data, splitChar=':'):
def parse_context(self, splitChar=':'):
"""
Values like

name: koen
age: 29

will be converted in a dict: {'name': 'koen', 'age': '29'}
will be converted in a dict:

{'name': 'koen', 'age': '29'}

and these lines are deleted from 'self.data'
"""

# make sure that the page context is only calculated once
if hasattr(self, "_page_context"):
return self._page_context
Copy link
Collaborator

Choose a reason for hiding this comment

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

Same as above


if not self.is_html():
return {}, data
return {}

values = {}
lines = data.splitlines()
lines = self._data.splitlines()
if not lines:
return {}, ''
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:
# Context lines start at the top of the file.
# The text start after the first empty line
# or at the first line without a colon
if not splitChar in line:
if not line:
i += 1
break

return values, '\n'.join(lines[i:])
key, value = line.split(splitChar, 1)
values[key.strip()] = value.strip()

self._data = '\n'.join(lines[i:])
Copy link
Collaborator

Choose a reason for hiding this comment

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

This alters _data, so it seems like we rely implicitly on parse_context being called to have the right page body in _data. Can you confirm? If so, this probably isn't desirable. You should load both _data and _page_context once.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

You are right, this changes _data. Maybe I should distinguish contentfrom data.

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 that you might as well parse the context when loading the page itself. There doesn't seem to be a use case for separating those two steps.

You could probably separate the whole page loading and parsing logic from the page object itself.

self._page_context = values
return values

def __repr__(self):
return '<Page: {0}>'.format(self.source_path)
31 changes: 15 additions & 16 deletions cactus/site.py
Original file line number Diff line number Diff line change
Expand Up @@ -267,6 +267,10 @@ def build(self):
else:
os.remove(path)

# One test / use case depends on
# the pages being read from disk for every built
self._build_page_list()
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 explain why those changes are needed?

I understand the changes in page.py (the whole page_context system is poorly engineered), but can you justify these as well, and why the two need to be in the same PR?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

tests.test_template_tags.TestMarkdown changes files and builds an existing Site five times. So the page list has to be read for every built.


# Render the pages to their output files
mapper = map
if self._parallel >= PARALLEL_AGGRESSIVE:
Expand Down Expand Up @@ -341,23 +345,18 @@ def pages(self):
"""
List of pages.
"""
if not hasattr(self, "_pages"):
self._build_page_list()
return self._pages

if not hasattr(self, "_page_cache"):
self._page_cache = {}

pages = []

for path in fileList(self.page_path, relative=True):

if path.endswith("~"):
continue

if not self._page_cache.has_key(path):
self._page_cache[path] = Page(self, path)

pages.append(self._page_cache[path])

return pages
def _build_page_list(self):
"""
Build the page list from the files found on disk
"""
self._pages = [
Page(self, path)
for path in fileList(self.page_path, relative=True)
if not path.endswith("~")]

def _rebuild_should_ignore(self, file_path):

Expand Down
6 changes: 6 additions & 0 deletions cactus/tests/data/colon-in-first-line.html
Original file line number Diff line number Diff line change
@@ -0,0 +1,6 @@
Author: Me
Title: Big Thing

Let's have one thing very clear: whatever.

Next Paragraph
31 changes: 29 additions & 2 deletions cactus/tests/test_basic.py
Original file line number Diff line number Diff line change
Expand Up @@ -5,6 +5,7 @@
from cactus.tests import SiteTestCase
from cactus.utils.filesystem import fileList
from cactus.utils.url import path_to_url
from cactus.page import Page


class TestSite(SiteTestCase):
Expand Down Expand Up @@ -48,7 +49,7 @@ def testRenderPage(self):

def testPageContext(self):
"""
Test that page context is parsed and uses in the pages.
Test that page context is parsed and used in the pages.
"""

shutil.copy(
Expand Down Expand Up @@ -113,4 +114,30 @@ def test_current_page(self):
self.assertEqual('True', f.read())

with open(os.path.join(self.path, '.build', other), 'rU') as f:
self.assertEqual('False', f.read())
self.assertEqual('False', f.read())


class TestPage(SiteTestCase):
Copy link
Collaborator

Choose a reason for hiding this comment

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

Does this need to be a separate class?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

It doesn't need to be a separate class. But I wanted to separate the more granular tests for Page from the tests for Site.

Copy link
Collaborator

Choose a reason for hiding this comment

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

Would be ideal to break out the other tests for Page into that class itself then (or into a separate file altogether).


def testPageContext(self):
"""
Test parsing of page context is parsed and used in the pages.
"""

shutil.copy(
os.path.join('cactus', 'tests', 'data', "koenpage-in.html"),
os.path.join(self.path, 'pages', 'koenpage.html')
)
page = Page(self.site, 'koenpage.html')
self.assertEqual(page.parse_context(),
{'name': 'Koen Bok', 'age': '29'})

shutil.copy(
os.path.join('cactus', 'tests', 'data', 'colon-in-first-line.html'),
os.path.join(self.path, 'pages', 'test.html')
)
page = Page(self.site, 'test.html')
self.assertEqual(page.parse_context(),
{u'Author': u'Me', u'Title': u'Big Thing'})
self.assertEqual(page._data,
"Let's have one thing very clear: whatever.\n\nNext Paragraph")