Page context - Githubissues

eudicots / Cactus

Static site generator for designers. Uses Python and Django templates.

BSD 3-Clause "New" or "Revised" License

3.47k stars 313 forks source link

Page context #140

Closed jammon closed 9 years ago

jammon commented 9 years ago

This changes the behavior of Site and Page in several subtle aspects.

Site builds the list of pages only when Site._build_page_list is called and not whenever Site.pages() is accessed.
Page reads the page file only once and not whenever Page.data() is accessed.
The content of the page is made a property of Page and is no more passed around as an argument.
Page.parse_context is changed to work more like I expected: The context data starts at the top of the file and the content starts after the first blank line or with the first line, that doesn't contain a colon.

krallin commented 9 years ago

Hi @jammon, and thanks for taking the time to contribute,

I commented on the times themselves, but here are more generic comments:

It looks like there are two set of changes here: page context and the page list. Do the two need to be bundled in the same PR?
Can you clarify what you mean by changing the page context to something closer to what you expected? What didn't work in the old system, why do you want to change it?

Note that we might not want to risk breaking compatibility with existing sites.

Cheers,

jammon commented 9 years ago

The changes on page_context build on having the page data as an attribute, and this means not reading it from the file whenever accessed, which was expected by the old behaviour of Site.pages().
I wrote a test case (tests/data/colon-in-the-first-line.html) that fails in the old system and fixed that behaviour. Now I even found a specification for the metadata extension of Python Markdown that describes what I had in mind.

krallin commented 9 years ago

Thanks,

I'll look into those changes into more detail over the weekend (don't have more time right now unfortunately).

Regarding the behavior change to the context system. Unfortunately there may be users relying on the old system, and I'm not big on just breaking backwards compatibility like this.

I understand your expectations might differ (and I'd personally share your POV here), and that the Python Markdown extension processes metadata differently, but that doesn't mean Cactus has to do so as well (we don't really use Python Markdown for starters).

I think the right approach here would be to break this system out of Page and into a plugin, and make it optional but enabled by default.

Thoughts?

jammon commented 9 years ago

The specs of markdown2 metadata extra look even different.

Putting the metadata / page context into a plugin is probably the right way.

krallin commented 9 years ago

I'll close this PR since you're working on #141 instead. Thanks!