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

Jump to bottom

Allow plugins to provide metadata #1603

Merged
merged 1 commit into from Feb 2, 2015
Merged

Allow plugins to provide metadata #1603

merged 1 commit into from Feb 2, 2015

Conversation

Carreau
Copy link
Contributor

@Carreau Carreau commented Jan 28, 2015

This allow plugins to provide a way to extract metadata from file the way
they wish to.

Metadata will be search first from the default value of as before this
PR. Metadata extracted by the plugin will then be merged into the
metadata dict, which in turn will be extended by the value in the
.meta file if present.

See start of discussion from #1602

I tried to get the api to extract metadata by way of plugins, as close as possible as other get_meta calls.
I also implement this for 1 plugin (.ipynb) the one I'm interested in.

This should not change current behavior for reading metadata from other places.

Kwpolska
Kwpolska reviewed Jan 28, 2015
View reviewed changes
nikola/post.py
@@ -136,6 +136,9 @@ def __init__(
self._dependency_uptodate_page = defaultdict(list)

default_metadata, self.newstylemeta = get_meta(self, self.config['FILE_METADATA_REGEXP'], self.config['UNSLUGIFY_TITLES'])
compiler_meta = self.compiler.read_metadata(self, self.config['FILE_METADATA_REGEXP'], self.config['UNSLUGIFY_TITLES'])
Copy link
Member

Choose a reason for hiding this comment

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

IMO, you should integrate this into get_meta instead.

@Carreau
Copy link
Contributor Author

Carreau commented Feb 1, 2015

Sorry for the time to respond, comment should be addressed.
moving things to get_meta seem a bit weird, also due to the cause that get_meta get passed self as first arg, but is not a method.

@Kwpolska
Copy link
Member

Kwpolska commented Feb 1, 2015

So what? read_metadata also gets the Post object, and also as self. read_metadata should not be put outside of the standard get_meta pipeline.

@Carreau Carreau force-pushed the plugin-meta branch 2 times, most recently from 738080d to 6cd634c Compare February 1, 2015 16:50
@Carreau
Copy link
Contributor Author

Carreau commented Feb 1, 2015

So what? read_metadata also gets the Post object, and also as self

No, i just ment that passing self as first argument just felt unnatural to me, I was thus thinking that from inside get_metadata I couldn't get access to self.compiler, but in fact I can, as it is name post.compiler from inside the function. Just me not used to the codebase, sorry if I express myself badly.

I've also forgot to push the cleanup commit that remove my debugs, and comments.
Tell me if you want me to squash everything into one commit.

Kwpolska
Kwpolska reviewed Feb 1, 2015
View reviewed changes
nikola/post.py
@@ -928,6 +928,8 @@ def get_meta(post, file_metadata_regexp=None, unslugify_titles=False, lang=None)
meta.update(_)

if meta:
compiler_meta = post.compiler.read_metadata(post, file_metadata_regexp, unslugify_titles, lang)
Copy link
Member

Choose a reason for hiding this comment

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

why is this still there?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Still ? You asked me to move it in it's in get_meta which I did. Though get_metareturn in two places, so if the post.compiler.read_metadata want to have an effect in all casses, I need to call it before each of the returns. Which explain why the call still appear twice in the diff.

@Kwpolska Kwpolska added this to the v7.3.1 milestone Feb 1, 2015
@Kwpolska Kwpolska self-assigned this Feb 1, 2015
@Kwpolska
Copy link
Member

Kwpolska commented Feb 1, 2015

  1. Failing tests: https://travis-ci.org/getnikola/nikola/jobs/49094459#L871 → dummy posts used for tests do not have a compiler attribute
  2. Failing flake8: https://travis-ci.org/getnikola/nikola/jobs/49094463#L136
  3. The part under if meta: should not be there; you should parse compiler meta only once in the code (don’t do it if an explicit .meta file exists? Refactor .meta loading?)
  4. Add this to CHANGES.txt (at the very top, under Features) and yourself to AUTHORS.txt.

@Carreau
Copy link
Contributor Author

Carreau commented Feb 1, 2015

Failing tests: https://travis-ci.org/getnikola/nikola/jobs/49094459#L871 → dummy posts used for tests do not have a compiler attribute
Failing flake8: https://travis-ci.org/getnikola/nikola/jobs/49094463#L136
The part under if meta: should not be there; you should parse compiler meta only once in the code (don’t do it if an explicit .meta file exists? Refactor .meta loading?)
Add this to CHANGES.txt (at the very top, under Features) and yourself to AUTHORS.txt.

Will do.

@Kwpolska
Copy link
Member

Kwpolska commented Feb 1, 2015

I recommend replacing the if meta: block with

if not meta:
    post.is_two_file = False

and then putting the compiler_meta code only in one place. It should also appear above the if lang is None: block. This sounds the most sensible to me.

@Carreau
Allow plugins to provide metadata.
90a005a 
This allow plugins to provide a way to extract metadata from file the way
they wish to.

Metadata will be search first from the default value of as before this
PR. Metadata extracted by the plugin will then be merged into the
metadata dict, which in turn will be extended by the value in the
`.meta` file if present.
@Carreau
Copy link
Contributor Author

Carreau commented Feb 1, 2015

Squashed, comment taken into account.
Does that looks better to you ?

Kwpolska
Kwpolska reviewed Feb 2, 2015
View reviewed changes
docs/extending.txt
@@ -374,6 +374,10 @@ They must provide:
If the compiler produces something other than HTML files, it should also implement ``extension`` which
returns the preferred extension for the output file.

These plugin can also be used to extract metadata from file. To do so, the
Copy link
Member

Choose a reason for hiding this comment

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

These plugins*

a dict containing the metadata*

Kwpolska added a commit that referenced this pull request Feb 2, 2015
@Kwpolska
Merge pull request #1603 from Carreau/plugin-meta
c87ffc6 
Allow plugins to provide metadata
@Kwpolska Kwpolska merged commit c87ffc6 into getnikola:master Feb 2, 2015
@Kwpolska
Copy link
Member

Kwpolska commented Feb 2, 2015

Code looks good, only needs some typo fixes in the documentation. Will fix myself.

Thanks for contributing!

@Kwpolska
Copy link
Member

Kwpolska commented Feb 2, 2015

(PS. I forgot to tell you squashing commits is unnecessary; we don’t really care.)

@Carreau Carreau deleted the plugin-meta branch February 2, 2015 16:54
@Carreau
Copy link
Contributor Author

Carreau commented Feb 2, 2015

Thanks for contributing!

Thanks for maintaining Nikola and guiding me through the codebase !

@ralsina ralsina mentioned this pull request Apr 22, 2015
Closed
@Kwpolska Kwpolska mentioned this pull request May 19, 2017
Closed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers
No reviews
Assignees

@Kwpolska Kwpolska

Labels
enhancement
Projects
None yet
Milestone
v7.3.1
Development

Successfully merging this pull request may close these issues.

None yet
2 participants
@Carreau @Kwpolska