sparkartgroup / gulp-markdown-to-json

Parse Markdown and YAML → compile Markdown to HTML → wrap it all up in JSON
MIT License
75 stars 14 forks source link

update frontmatter error handling #13

Closed thecotne closed 8 years ago

pushred commented 8 years ago

Thanks for the PR!

Yeah, I can see front-matter blowing up easily as things stand. YAML is pretty lenient so I haven't run into it in my own use, but js-yaml throws on all but warnings. It will throw on those as well if you don't provide a function.

I hate to handle this with a try/catch, perhaps a better first step is a PR for front-matter to passthrough options to js-yaml to specify an onWarning handler.

Do you have some example content where you're managing to generate a js-yaml exception? Just curious. Will have to see what js-yaml considers to be warnings vs. exceptions.

thecotne commented 8 years ago

since this is backwards compatible why not merge it? you can improve code quality as you like in future

i personally prefer handling errors with try/catch

i have non developer writing some posts in md files so i got plenty of examples of yaml errors

for example

---
tooltip: 111
tooltip: 222
---

will result in [YAMLException]: duplicated mapping key

---
tooltip: "111
---

this is [YAMLException]: unexpected end of the stream within a double quoted scalar

---
tooltip: "some title that has some "quoted text" in it"
---

this is [YAMLException]: can not read a block mapping entry; a multiline key may not be an implicit key

without correct error reporting it is impossible to track which file is broken

pushred commented 8 years ago

Gotcha, I hear you on the end user side. I can try to chase down a better solution in front-matter/js-yaml as I work on the 1.0 for this plugin. I'm fine with the try/catch in the interim but there's other issues here:

TypeError: Cannot read property 'line' of undefined`

It's probably simplest if I just implement error handling as part of 1.0, it should be ready next week.

pushred commented 8 years ago

Alrighty I'm now catching and emitting errors with https://github.com/SparkartGroupInc/gulp-markdown-to-json/commit/b1af13ab65c1ec1818803eb17e9df0036d259ccb

Unfortunately the error reporting isn't as clean as what you implemented because I didn't want to implement parsing for a particular dependency. In this case front-matter actually switched to js-yaml but this PR was written against it's earlier release that used yaml-js.

Also, plugins are wrapped in PluginError for interop with other gulp error reporting plugins. So won't be pretty if you're logging out it out directly.

thecotne commented 8 years ago

@pushred any progress on this?

pushred commented 8 years ago

Thanks for following up! I addressed this in #14. I thought I had a lingering issue but testing again today it was unrelated to usage with gulp. So I went ahead and published 1.0.0. Please have a look at the release notes and give it a shot.