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

[WIP] Conversion plugin for Nikola's #2761: conversion plugin for pre-v8 tag-style metadata #271

Merged
merged 9 commits into from May 5, 2018

Conversation

@felixfontein
Copy link
Contributor

@felixfontein felixfontein commented May 2, 2018

Companions #3068.

@felixfontein
Copy link
Contributor Author

@felixfontein felixfontein commented May 2, 2018

Ok, first working version.

Loading

@felixfontein felixfontein requested a review from Kwpolska May 2, 2018
@felixfontein
Copy link
Contributor Author

@felixfontein felixfontein commented May 2, 2018

(requires getnikola/nikola#3068 to work)

Loading

if extractor.name == 'nikola':
final_str = meta_str + '\n\n' + content_str
elif extractor.name == 'yaml':
final_str = meta_str + '\n---\n' + content_str
Copy link
Member

@Kwpolska Kwpolska May 2, 2018

Choose a reason for hiding this comment

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

Is there a way not to hardcode this?

Loading

Copy link
Contributor Author

@felixfontein felixfontein May 2, 2018

Choose a reason for hiding this comment

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

We could extend the metadata extractors so that they offer a recombine function which does this. That would avoid this. If this is ok for you, I'll start a PR for that.

Loading

Copy link
Member

@Kwpolska Kwpolska May 2, 2018

Choose a reason for hiding this comment

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

We’ve got utils.write_metadata for that. Also handles adding <!-- HTML comments --> where necessary.

Loading

Copy link
Contributor Author

@felixfontein felixfontein May 2, 2018

Choose a reason for hiding this comment

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

But that only writes metadata, right? And doesn't include the post's content?

Loading

Copy link
Member

@Kwpolska Kwpolska May 2, 2018

Choose a reason for hiding this comment

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

Yes, but then you can just fd.write(content) without worrying about anything, cf. any compiler plugin.

Loading

Copy link
Contributor Author

@felixfontein felixfontein May 2, 2018

Choose a reason for hiding this comment

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

Aaah, I missed that part :)

Loading

Copy link
Contributor Author

@felixfontein felixfontein May 2, 2018

Choose a reason for hiding this comment

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

Done. Two drawbacks are:

  1. might use another metadata format (which screws up metadata transformations);
  2. adds HTML comments everywhere, even if they haven't been there before and aren't needed.

Loading

Copy link
Member

@Kwpolska Kwpolska May 3, 2018

Choose a reason for hiding this comment

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

You could fix 2. by checking if the format is reST.

Loading

Copy link
Contributor Author

@felixfontein felixfontein May 3, 2018

Choose a reason for hiding this comment

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

I don't understand how you mean this.

For 1., how about adding an argument like force_format to write_metadata which, if set to True, bails out (by returning None or raising an exception) if the given / selected metadata format cannot be used.

Loading

L.warn('Cannot convert {0} (language {1}): a metadata value mapping is defined for "{2}"!'.format(fname, lang, meta_key))

# Update metadata
updated = False
Copy link
Member

@Kwpolska Kwpolska May 2, 2018

Choose a reason for hiding this comment

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

Feature request:

if section is present, but category isn’t: set category to section value and remove section
if both are present: show a warning (as is done on the nuke-sections branch of the main repo)

Loading

Copy link
Contributor Author

@felixfontein felixfontein May 2, 2018

Choose a reason for hiding this comment

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

Should this be part of the main functionality, or should it be possible to select one of the two functionalities (kill special tags, and kill sections)? I would make it possible to do only one of them, since (at least for sections) you could still use them with a custom taxonomy plugin. So some people might not want to remove them (but still convert special tags).

Loading

Copy link
Member

@Kwpolska Kwpolska May 2, 2018

Choose a reason for hiding this comment

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

Main functionality. Plan for the 99%, the rest can handle it on their own.

Loading

Copy link
Contributor Author

@felixfontein felixfontein May 2, 2018

Choose a reason for hiding this comment

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

Sure. Disabling the corresponding code isn't hard, either.

Loading

Copy link
Contributor Author

@felixfontein felixfontein May 2, 2018

Choose a reason for hiding this comment

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

Implemented in 685be9c.

Loading

@ralsina
Copy link
Member

@ralsina ralsina commented May 2, 2018

+1, not familiar with the section killing, so going with Kw's opinion there.

Loading

@felixfontein
Copy link
Contributor Author

@felixfontein felixfontein commented May 3, 2018

Now the tests also no longer fail, thanks to 42d6e2b.

Loading

@felixfontein
Copy link
Contributor Author

@felixfontein felixfontein commented May 4, 2018

@Kwpolska how do you want to proceed with this one?

Loading

@felixfontein
Copy link
Contributor Author

@felixfontein felixfontein commented May 4, 2018

(I guess for a v8 beta, this one should be available.)

Loading

flagged.append(post)
if flagged:
if len(flagged) == 1:
L.info('1 post (and/or its translations) contains old-style metadata or have section metadata:')
Copy link
Member

@Kwpolska Kwpolska May 4, 2018

Choose a reason for hiding this comment

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

you mixed up have/has in both cases

Loading

Copy link
Contributor Author

@felixfontein felixfontein May 4, 2018

Choose a reason for hiding this comment

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

Thanks! Fixed.

Loading


# Recombine metadata with post text if necessary, and write back to file
meta_str = utils.write_metadata(meta, metadata_format=extractor.name, compiler=post.compiler,
comment_wrap=True, site=self.site)
Copy link
Member

@Kwpolska Kwpolska May 4, 2018

Choose a reason for hiding this comment

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

comment_wrap=(post.compiler.name != 'rest')

Loading

Copy link
Contributor Author

@felixfontein felixfontein May 4, 2018

Choose a reason for hiding this comment

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

Ah, ok. I'm using that now. Doesn't make me 100% happy, but I guess it is fine for > 99% of the time ;)

Loading

@Kwpolska
Copy link
Member

@Kwpolska Kwpolska commented May 4, 2018

Did you test this with YAML or other tricky edge cases?

Loading

@felixfontein
Copy link
Contributor Author

@felixfontein felixfontein commented May 4, 2018

Tags as list work now. What doesn't work are two-file posts, it seems. They don't work, because no extractor is stored for them. get_metadata_from_meta_file simply doesn't return it. Is this intentional?

Loading

@felixfontein
Copy link
Contributor Author

@felixfontein felixfontein commented May 4, 2018

(YAML and TOML work fine, at least outside two-files.)

Loading

@Kwpolska
Copy link
Member

@Kwpolska Kwpolska commented May 4, 2018

Fixed on master (getnikola/nikola@c2044a2)

Loading

@felixfontein
Copy link
Contributor Author

@felixfontein felixfontein commented May 4, 2018

LOL, you were faster. I just pushed a branch for that :)

Loading

@felixfontein
Copy link
Contributor Author

@felixfontein felixfontein commented May 4, 2018

Now two-file posts work as well. METADATA_MAPPING support also seems to work.

Loading

@felixfontein
Copy link
Contributor Author

@felixfontein felixfontein commented May 4, 2018

I think it should be fine now.

Loading

@Kwpolska Kwpolska merged commit c0918c7 into master May 5, 2018
2 checks passed
Loading
@Kwpolska Kwpolska deleted the plugin-for-2761 branch May 5, 2018
@Kwpolska
Copy link
Member

@Kwpolska Kwpolska commented May 5, 2018

There were two bugs I found while fixing chriswarrick.com:

  1. saving as UTF-8 with BOM is evil
  2. posts without translations crash:
Traceback (most recent call last):
  File "/Users/kwpolska/virtualenvs/nikola/lib/python3.6/site-packages/doit/doit_cmd.py", line 172, in run
    return command.parse_execute(args)
  File "/Users/kwpolska/virtualenvs/nikola/lib/python3.6/site-packages/doit/cmd_base.py", line 127, in parse_execute
    return self.execute(params, args)
  File "/Users/kwpolska/git/nikola/nikola/plugin_categories.py", line 147, in execute
    return self._execute(options, args)
  File "/Users/kwpolska/website/plugins/upgrade_metadata_v8/upgrade_metadata_v8.py", line 105, in _execute
    with io.open(fname, "r", encoding="utf-8-sig") as meta_file:
FileNotFoundError: [Errno 2] No such file or directory: 'posts/2017/09/02/spawning-subprocesses-smartly-and-securely.pl.rst'

I fixed both in commit f79b5c5.

Loading

@felixfontein
Copy link
Contributor Author

@felixfontein felixfontein commented May 5, 2018

Ah, that's what -sig is for. I copied the open command from post.py (and changed it from read to write), and forgot to think about the encoding specified there... Sorry for that!

Thanks for fixing this.

Loading

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

Successfully merging this pull request may close these issues.

None yet

3 participants