wooorm / markdown-rs

CommonMark compliant markdown parser in Rust with ASTs and extensions
https://docs.rs/markdown/1.0.0-alpha.21/markdown/
MIT License
906 stars 50 forks source link

Add support for serializing trees as JSON #37

Closed kyle-mccarthy closed 1 year ago

kyle-mccarthy commented 1 year ago

Description

This is a continuance of PR #30 that leverages serde's macros instead of explicit impls for serialize.

Closes

Tests

Test have been taken from the mdast-util-from-markdown JS package. Tests that were failing have been #[ignore]d.

kyle-mccarthy commented 1 year ago

One more thing, is that I still think it’s good to have a build script, to take test fixtures, and generate test cases, like what you had inline before...

I am totally open to doing this; however, I am not sure how to handle:

wooorm commented 1 year ago

the files that I've manually had to change to account for utf-8

The input is UTF-8, so I don’t understand why you need to account for this?

the failing test cases that have been ignored using the ignore macro

I’d go about this as follows: you could use this project itself in that build script to check if actual is expected, in which case you generate a normal test. Otherwise, you generate a skipped test: https://doc.rust-lang.org/book/ch11-02-running-tests.html#ignoring-some-tests-unless-specifically-requested

kyle-mccarthy commented 1 year ago

Hey so I have a couple of concerns:

Alternatively,I could just remove the tests?

wooorm commented 1 year ago

a) this is no longer the case since https://github.com/wooorm/markdown-rs/commits/main b) it doesn’t run all the time anymore and the output is checked in so visible c) you could remove positional info before testing d) I am also okay with removing most tests. I don’t believe the core of the issue you are solving here is about matching the exact output of mdast-util-from-markdown

kyle-mccarthy commented 1 year ago

@wooorm ah nice I didn't realize that the test situation changed! that said, I went with option d since I agree that the tests I added were overkill for this feature.

wooorm commented 1 year ago

Thanks for your hard work on this feature!!