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

Generalize metadata functions and moving them to utils.py #2856

Merged
merged 8 commits into from Jul 1, 2017

Conversation

@felixfontein
Copy link
Contributor

@felixfontein felixfontein commented Jul 1, 2017

This makes implementing getnikola/plugins#232 much easier, and separates the metadata extraction from the higher level parts (metadata transformation).

@getnikola getnikola deleted a comment Jul 1, 2017
@getnikola getnikola deleted a comment Jul 1, 2017
@getnikola getnikola deleted a comment Jul 1, 2017
@getnikola getnikola deleted a comment Jul 1, 2017
@getnikola getnikola deleted a comment Jul 1, 2017
@getnikola getnikola deleted a comment Jul 1, 2017
Copy link
Member

@Kwpolska Kwpolska left a comment

I wanted a different kind of refactor in #2380, this would kind-of break it IMO.

elif not match and result:
return (result[0][0], result[0][1].strip())
else:
return (None,)

This comment has been minimized.

@Kwpolska

Kwpolska Jul 1, 2017
Member

return None, None — and no parentheses above

This comment has been minimized.

@felixfontein

felixfontein Jul 1, 2017
Author Contributor

Fine for me. I just copy'n'pasted this code from post.py and didn't modify anything in this function.

* ``'rest'``: metadata in reST format (the standard Nikola
reST-like metadata format)
"""
if data.startswith('---'): # YAML metadata

This comment has been minimized.

@Kwpolska

Kwpolska Jul 1, 2017
Member

There should be something to split metadata based on the post object as well, and the post object should know what metadata type it was.

This comment has been minimized.

@felixfontein

felixfontein Jul 1, 2017
Author Contributor

That would be for #2830, right?

This comment has been minimized.

@Kwpolska

Kwpolska Jul 1, 2017
Member

Yes, I’ll handle it then.

@felixfontein
Copy link
Contributor Author

@felixfontein felixfontein commented Jul 1, 2017

I think you mean #2830, not #2380.

I actually forgot about #2380; I just wanted to convert the existing functionality to something which can be easier reused.

@getnikola getnikola deleted a comment Jul 1, 2017
@Kwpolska
Copy link
Member

@Kwpolska Kwpolska commented Jul 1, 2017

Yeah, that was a typo: #2830. I’ll go 👍 for merging it, but don’t expect this API to be stable.

return '', split_result[0]
# ['metadata', '\n\n', 'post content']
return split_result[0], split_result[-1]
meta, content, _ = split_metadata(data)

This comment has been minimized.

@felixfontein

felixfontein Jul 1, 2017
Author Contributor

I don't understand this one. Is codacy maybe confusing the imported utils.split_metadata with PageCompiler.split_metadata?

This comment has been minimized.

@Kwpolska

Kwpolska Jul 1, 2017
Member

It seems to be. Ignore that warning.

@felixfontein
Copy link
Contributor Author

@felixfontein felixfontein commented Jul 1, 2017

Sure. I assume nothing in master is stable until the first v8 version (or release candidate) is released :)

I'm not sure whether I'll use this API though, now that you mentioned #2830...

if meta[k] is None:
meta[k] = ''
meta, metadata_type = utils.extract_metadata(file_lines)
if metadata_type == 'yaml':

This comment has been minimized.

@Kwpolska

Kwpolska Jul 1, 2017
Member

For both blocks:

      if metadata_type in ('yaml', 'toml'):
          # Map metadata from other platforms to names Nikola expects (Issue #2817)
          map_metadata(meta, metadata_type, config)

This comment has been minimized.

@felixfontein

felixfontein Jul 1, 2017
Author Contributor

Fixed.



# Moved to global variable to avoid recompilation
# of regex every time re_meta() is called.

This comment has been minimized.

@Kwpolska

Kwpolska Jul 1, 2017
Member

Regex fun fact: there’s a cache, for 512 entries if memory serves. It probably doesn’t get recompiled. (Don’t change this back)

This comment has been minimized.

@felixfontein

felixfontein Jul 1, 2017
Author Contributor

Ah, I didn't knew about that cache. I don't like the idea of depending on it in such cases where it is obvious that the regex will be used over and over again, though, so yes, I won't change this back :)

@getnikola getnikola deleted a comment Jul 1, 2017
Copy link
Member

@Kwpolska Kwpolska left a comment

LGTM

@getnikola getnikola deleted a comment Jul 1, 2017
@felixfontein felixfontein merged commit 74b96ee into master Jul 1, 2017
1 of 5 checks passed
1 of 5 checks passed
continuous-integration/appveyor/branch Waiting for AppVeyor build to complete
Details
continuous-integration/appveyor/pr Waiting for AppVeyor build to complete
Details
continuous-integration/travis-ci/pr The Travis CI build is in progress
Details
continuous-integration/travis-ci/push The Travis CI build is in progress
Details
codacy/pr Good work! A positive pull request.
Details
@felixfontein felixfontein deleted the generalize-metadata-functions branch Jul 1, 2017
@felixfontein
Copy link
Contributor Author

@felixfontein felixfontein commented Jul 1, 2017

Thanks for reviewing!

Kwpolska added a commit that referenced this pull request Jul 3, 2017
Signed-off-by: Chris Warrick <kwpolska@gmail.com>
ralsina added a commit that referenced this pull request Jul 9, 2017
* Rename UNSLUGIFY_TITLES → FILE_METADATA_UNSLUGIFY_TITLES

Signed-off-by: Chris Warrick <kwpolska@gmail.com>

* Initial scaffolding for metadata extractor plugins

Signed-off-by: Chris Warrick <kwpolska@gmail.com>

* Use correct environment marker syntax

Signed-off-by: Chris Warrick <kwpolska@gmail.com>

* Style fixes

Signed-off-by: Chris Warrick <kwpolska@gmail.com>

* Use metadata extractor APIs and plugins

Signed-off-by: Chris Warrick <kwpolska@gmail.com>

* Fix galleries; add basic split support (temporary)

Signed-off-by: Chris Warrick <kwpolska@gmail.com>

* Fix compatibility with .meta files

Signed-off-by: Chris Warrick <kwpolska@gmail.com>

* #2856 was effectively undone

Signed-off-by: Chris Warrick <kwpolska@gmail.com>

* Make metadata splitting work

Signed-off-by: Chris Warrick <kwpolska@gmail.com>

* Code cleanups

Signed-off-by: Chris Warrick <kwpolska@gmail.com>

* Fix tests, add new config_present condition

Signed-off-by: Chris Warrick <kwpolska@gmail.com>

* Allow whitespace before Nikola-style metadata

Signed-off-by: Chris Warrick <kwpolska@gmail.com>

* Fix RSS tests

Signed-off-by: Chris Warrick <kwpolska@gmail.com>

* Add MetadataExtractors documentation and `override` MetaPriority

Signed-off-by: Chris Warrick <kwpolska@gmail.com>

* Style fixes

Signed-off-by: Chris Warrick <kwpolska@gmail.com>

* Add tests for metadata extractors

Signed-off-by: Chris Warrick <kwpolska@gmail.com>

* Turns out we run flake8 on tests, too

Signed-off-by: Chris Warrick <kwpolska@gmail.com>

* Make write_metadata work with metadata extractors

Signed-off-by: Chris Warrick <kwpolska@gmail.com>

* Fix #2830 — add MetadataExtractor plugins

Signed-off-by: Chris Warrick <kwpolska@gmail.com>

* Minor documentation fixes [ci skip]

Signed-off-by: Chris Warrick <kwpolska@gmail.com>

* Address review by @felixfontein

Signed-off-by: Chris Warrick <kwpolska@gmail.com>

* Remove useless comment [ci skip]

Signed-off-by: Chris Warrick <kwpolska@gmail.com>

* Slightly smarter return values for split_metadata_for_text

Signed-off-by: Chris Warrick <kwpolska@gmail.com>

* Pass site when writing metadata in importers

Note this is a minor API change (removes @staticmethod) that needs to be
reflected in some importers.

cc @felixfontein

Signed-off-by: Chris Warrick <kwpolska@gmail.com>

* Document potentially confusing split_metadata_from_text behavior

Signed-off-by: Chris Warrick <kwpolska@gmail.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Linked issues

Successfully merging this pull request may close these issues.

None yet

2 participants