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 #140

Closed
wants to merge 5 commits into from
Closed

Page context #140

wants to merge 5 commits into from

Conversation

jammon
Copy link
Contributor

@jammon jammon commented May 2, 2015

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.

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

@krallin
Copy link
Collaborator

krallin commented May 2, 2015

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

jammon commented May 2, 2015

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

krallin commented May 2, 2015

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

jammon commented May 2, 2015

The specs of markdown2 metadata extra look even different.

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

@krallin
Copy link
Collaborator

krallin commented May 13, 2015

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

@krallin krallin closed this May 13, 2015
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants