Fix #58: Rename json feature to serde

[barafael]

Also removes unused dependency on serde-json.

Note, this is a breaking change due to the rename.

Motivation: serde is a conventional and idiomatic feature name for serde support in other crates, too. Serde can do more than json, so the feature name should not be too specific IMO.

[wooorm]

• Not too inclined to do a major for this, although, at this point we’re not at a stable release so it doesn’t matter much and I don’t think the feature is used much
• json does kinda cover, well, the fact that the AST is guaranteed to be serializable through JSON, not through anything else (see https://github.com/syntax-tree/unist#where-this-specification-fits). The goal of this feature is JSON. Not arbitrary Serde things. I’m not really opposed to serde, but json kinda covers the intention.

Thoughts? If you feel strongly about the current state that’s fine.

[barafael]

OK. In any case, the dependency on serde_json is not required and should be removed. Feel free to close this PR.

[wooorm]

I’d prefer to merge this PR if possible. With the original feature request: drop the unused dependency. That’s great to have!

If you want to introduce a breaking change, and feel strongly about doing so because it’s better, that’s fine too, I’m generally not too into breaking things though, hence my previous points. Or we can make that a separate feature request, a separate discussion.

[barafael]

Another suggestion: we could keep the json feature, and add a serde feature that enables it automatically. This mitigates the breaking change. Additionally, we could add an mdast feature, which also simply enables the serde feature. What do you think?

[wooorm]

This mitigates the breaking change

Mitigating that is a great idea!
Do you want to allow and document both?
Or is this a soft deprecation, where you want json to be removed in the future, and only serde is documented?

Additionally, we could add an mdast feature, which also simply enables the serde feature

3 features for the same thing seems a bit much.
And, mdast seems confusing to me: mdast is always enabled. You can use to_mdast without serde fine?

[barafael]

I mean, you're the boss :D
I'm thinking the serde feature could be aliasing the json feature. No duplicate docs required. I'm not sure what you mean by 'allow'ing it...
Removing the json feature in any case is a breaking change, which as you said is better to be avoided if possible, and this is not a super harmful one.

[mickvangelderen]

@wooorm technically it is allowed to make breaking changes while you're in the alpha release still right? As a library consumer, when you depend on a crate that is at version 0.3.0 and you install 1.0.0-alpha.x you accept the risk that there may be breaking changes.

Still, keeing the json feature might be appreciated. At the same time, you will be stuck with this non-canonical json feature until version 2.0.0. I would consider doing an as clean as possible 1.0.0 release.

[wooorm]

You are correct that there are no semver guarantees before 1.0.0!
But, as you point out, you don’t need to break things every release before 1.0.0 either.
The next release is not 1.0.0, so there might not be a “being stuck until 2.0.0”, and the 1.0.0 release can be “as clean as possible”.

---

I'm thinking the serde feature could be aliasing the json feature. No duplicate docs required. I'm not sure what you mean by 'allow'ing it...

If both are allowed (as in, they do things, they are implemented), then either: a) both need to be documented, or: b) only the preferred one needs to be documented.
I am fine with either.

[wooorm]

@barafael Can you update the PR to implement both and optionally document both?

[mickvangelderen]

The next release is not 1.0.0, so there might not be a “being stuck until 2.0.0”, and the 1.0.0 release can be “as clean as possible”.

Do you mean that the next release will be another pre-release like 1.0.0-alpha.x? When is the 1.0.0 release planned, or what are the criteria that must be satisfied before the 1.0.0 release is ready?

[wooorm]

Correct. Criteria:

a) get more people to use and test this
b) improved AST: #27 (comment)
c) likely builder patterns for options: #40

[mickvangelderen]

a) get more people to use and test this

Perhaps releasing a 0.4.0 can help so that the crates.io readme and https://docs.rs/markdown get updated. The amount and quality of documentation are, I believe, pretty good. However, you need to land on the github page to find it.

[wooorm]

You are correct but the previous author made me promise not to do that. :'(

[barafael]

@wooorm done.

BTW, as the Readme of this project is fairly substantial, what do you think about adding #![doc = include_str!("readme.md")]?

[wooorm]

doc = include_str

That might perhaps be useful!
Normally I document much more in the readme.
Due to rust having docs.rs, that’s not in the readme, which is now more focussed on the project.

Would there be a real benefit in doing so though? Wouldn’t having the same info in two places make it harder to find things?

Do you know of more projects that include the readme in their API docs?

[barafael]

there's a bunch, unsorted though:
https://github.com/search?q=%23%21%5Bdoc+%3D+include_str%21%28%22readme.md%22%29%5D+language%3ARust&type=code&ref=advsearch
I just wanted to point it out as the readme is substantial, out of scope for this issue of course.

[wooorm]

I just wanted to point it out as the readme is substantial, out of scope for this issue of course.

Thanks for the search. From the numbers, it doesn’t seem super common though.

I’m inclined to keep API docs on docs.rs, and keep other info in the readme, so as not to duplicating info in different places

[wooorm]

Thanks, released! <3