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

Allow plugins to provide metadata #1603

Merged
merged 1 commit into from Feb 2, 2015
Merged

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.

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

@Kwpolska Kwpolska Jan 28, 2015

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 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 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 Feb 1, 2015
@Carreau
Copy link
Contributor Author

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

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

@Kwpolska Kwpolska Feb 1, 2015

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

@Carreau Carreau Feb 1, 2015

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

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 Carreau commented Feb 1, 2015

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

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

@Kwpolska Kwpolska Feb 2, 2015

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 issue Feb 2, 2015
Allow plugins to provide metadata
@Kwpolska Kwpolska merged commit c87ffc6 into getnikola:master Feb 2, 2015
2 checks passed
@Kwpolska
Copy link
Member

@Kwpolska 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 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 Feb 2, 2015
@Carreau
Copy link
Contributor Author

@Carreau Carreau commented Feb 2, 2015

Thanks for contributing!

Thanks for maintaining Nikola and guiding me through the codebase !

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants