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

Conversation

jammon
Copy link
Contributor

@jammon jammon commented May 3, 2015

Ripped parsing of page context out of Page and put it into a plugin. With tests and documentation.
There are different styles of metadata, the plugin tries to guess what is appropriate.
Multimarkdown style is implemented but not used. In the future the style should be up for configuration.

@krallin
Copy link
Collaborator

krallin commented May 7, 2015

Hi Johannes,

Just a quick note that i'll try to take a look at this over the weekend — sorry about the delayed response here.

Cheers,

@krallin krallin self-assigned this May 7, 2015
@jammon
Copy link
Contributor Author

jammon commented May 7, 2015

I appreciate your work for this project very much, and have learned to be patient in open source development, since I'm well aware, that there are more pressing issues in everybodies life than such a project.
So take your time :)

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

@krallin
Copy link
Collaborator

krallin commented May 9, 2015

Hey @jammon ,

I added some inline comments. Most of them are rather minor. The two major things are:

  • I'd rather you use preBuildPage in your tests
  • We need to make sure this plugin runs first

Cheers,

@jammon
Copy link
Contributor Author

jammon commented May 10, 2015

Thank you for in-depth comments, I agree with all of them.
The plugin now has ORDER = 0. Will that be low enough?

@krallin krallin mentioned this pull request May 13, 2015
@krallin
Copy link
Collaborator

krallin commented May 20, 2015

Hey @jammon

Just a quick note that this week / the next one have / are going to be very busy on my end. I'll try to review this but I can't promise anything just yet.

Cheers,

@jammon
Copy link
Contributor Author

jammon commented May 25, 2015

@krallin No problem, currently I have more than enough work to do as well.

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