Have Meta extension support YAML #372
Conversation
|
I have a number of concerns about this. First, the existing tests are failing. That needs to be fixed. And where's the tests and documentation of the few features? But before we get to that, why can't this be a seperate extension? And why would I use YAML to define structured data and then throw all that away? The existing meta-data extension has no idea of data types and that's why it returns everything as a list of strings. But with YAML I get type inference that works for free. If I'm using YAML, I want to keep that. Seems to me a seperate extension with a different API makes more sense. That said, I realize that various existing projects (Pelican, MkDocs, etc.) use the existing extension and (type ignorant) API. I suppose one might be interested in using YAML with such a framework without the need to hack the framework, which this would accomplish. That said, if after carefully crafting my YAML meta-data with a give type, that type is thrown out and a different type is assigned by the framework, I would find that extremely annoying. I could go either way on this. In fact, as I have recently been playing with some of these frameworks that use the existing extension, I was thInking that it might be useful to provide a way to assign a given type to a specific name. Perhaps when loading the extension, a set of key-value pairs could be passed in as configs that define a type for given key names in the meta data. Something like |
|
After thinking on this, I believe the best approach is to follow MultiMarkdown's lead and optionally allow the YAML deliminators ( I suppose we could also add a config option to turn on YAML parsing which should return the data as YAML parses it. But the default would be to have that feature turned off (for backward compatibility) even when the YAML deliminators are present. That way, existing frameworks will continue to work as-is, and their users could even add the YAML deliminators (which cause the metadata to get rendered as a table by GitHub for example). And, if any framework actually wants to have real YAML parsing, they can turn on the YAML parsing and get structured data. In fact, I like that better that providing an API to define types for the current implementation (as I suggested in my previous comment). if you want structured data, turn on YAML and if you want raw data (the current API), turn off YAML---either way we should still support the YAML deliminators. |
kernc
force-pushed
the
meta_yaml
branch
2 times, most recently
from
884de30 to
499cea2
Compare
|
Ok, I agree to what you are saying about retaining structure. Took me 40 minutes yesterday to figure out it's by design I can't force- So, is something like this (PR updated) what you had in mind? The dependency on PyYAML is a soft one, required only if YAML is enabled. |
|
Not exactly. You added support for YAML, but you did not add support for the YAML deliminators when YAML is not turned on---which should just be a couple more lines of code. Also, let's not import yaml within the method. I'd prefer to import at the module level and if it fails assign try:
import yaml
except ImportError:
yaml = False
# then later in the method:
if self.config['yaml'] and yaml ...Also, lets call the config key 'yaml', no reason to make it so lengthy. |
|
So if YAML is not turned on, what happens, the YAML header is just skipped? And regarding if self.config['yaml'] and yamlIf YAML is requested but |
|
Oh, I get it. You want to enable just skipping Update: But what if a YAML header, say a multi-level or a list example of it, is unparsable as simple meta-data (when parsing YAML is disabled)? |
|
The contents of a document should never cause the parser to raise an error (we can't be crashing web servers---the libraries most common use case). Your implementation would raise an error only when a document contained YAML but the yaml lib was not available. However, the error would only occur when trying to parse the document (not on class initialization). Using my suggested method, the parser doesn't raise an error---the results are just not what was expected. Perhaps not as easy to debug---but good documentation should address that. |
|
Ok, how about now? Not crashing anywhere, only a warning is logged iff YAML parsing is enabled and a YAML-header'd document is encountered and |
markdown/extensions/meta.py
Outdated
There was a problem hiding this comment.
Shouldn't we be passing the configs into the Class on initiation? Like this.
There was a problem hiding this comment.
Dunno. Inspecting all the built-in extensions, I've seen examples of both, and more. This way, we avoid one thin __init__(). Since this isn't functional programming and since MetaPreprocessor and MetaExtension are tangled one way or another, doesn't really matter IMHO. Unless you have a preference. ¯_(ツ)_/¯
There was a problem hiding this comment.
Yes, this is my preference. It makes for easier to read code later and it is the way the API is designed to work. If other extensions don't follow this pattern, they should be updated--unless there is a good reason not to of course.
There was a problem hiding this comment.
Done. FTR, the given wikilinks example sets md attribute exactly the same way:
wikilinkPattern.md = md
|
Whaddya think? 😃 |
tests/test_extensions.py
Outdated
There was a problem hiding this comment.
Let's leave this test as-is and add a seperate test with the YAML deliminators. That way we have a test for each possible scenario.
1 similar comment
tests/test_extensions.py
Outdated
There was a problem hiding this comment.
Sorry, I missed this before. This way of passing config options into extensions is pending deprecation. I realize some existing tests still use this, but let's not add any more. We'll just have to change it before the next release anyway.
There was a problem hiding this comment.
Oh, no problem. Hope now is fine?
|
@kernc thanks for the work on this. I just merged your work with a couple minor changes. |
|
Yay, thanks! 😃 |
3 participants
The Meta extension currenly only supports extended RFC822-style headers.
Another ubiquitous meta-data format, even prevalent perhaps when it comes to *.md files, is YAML. Pandoc supports YAML metadata, Jekyll uses it.
It'd be nice if this markdown implementation also supported YAML meta data. Making a plugin is an option, but the only existing plugin seems to not work at all and break the established API.
It'd make some sense to have YAML meta-data supported out-of-the-box. Please let me know what you think. With heads up, I'll be happy to further write tests and document the addition.