search

the-alchemists-of-arland / gray-matter-rs

A tool for easily extracting front matter out of a string. It is a fast Rust implementation of gray-matter. Parses YAML, JSON, TOML and support for custom parsers. Use it and let me know by giving it a star!
https://docs.rs/gray_matter
MIT License
37 stars 4 forks source link

Implement `Deserializer` for `Pod` #5

Open kmaasrud opened 3 years ago

kmaasrud commented 3 years ago

While investigating #3, I realised that we don't really have a need for the polyglot datatype Pod. Since we nonetheless coerce it into a serde_json::Value upon deserializing, Pod can easily be replaced by Value - essentially skipping a conversion step.

This would increase performance (probably not by much, but every nanosecond counts) and drastically reduce code complexity.

Changing this would introduce an API change, so I don't see the rush to do it, but I'm leaving this issue here for whenever we deem it fit to implement.

yuchanns commented 3 years ago

Well, when I wrote this crate, I was expecting a scenario where the frontmatter of markdown is parsed out at compile time so that yew can generate static files, which is similar to what vuepress does. So I didn't care about performance, since it's a compile-time overhead.

However, I have noticed that some people use it at runtime.

So I general totally agree with you that a little bit of runtime performance optimization is needed.

But as to whether discarding pod types to improve performance, some benchmark comparing this two way shall be done to make it more convincing.

When the pod type was introduced into the crate, I hadn't considered serd_json so I didn't use value instead.

yuchanns commented 3 years ago

Also, I think we should do optimization as soon as it is proven necessary. The fewer users, the less cost to make breaking changes. Just follow semantics well.

kmaasrud commented 3 years ago

Thinking more about it, perhaps we should keep Pod after all! It's not a bad thing to be in control of our own datatype (if we for instance want to extend into supporting TOML's datetimes).

The deserialize function is essentially what needs to be changed. Namely, we need to implement Deserialize on Pod to avoid the need for a conversion into serde_json::Value. This wouldn't introduce an API change either :smile:

yuchanns commented 3 years ago

Yes, we should think about this a bit more carefully. Currently using serde_json to do serialization and deserialization is just for the sake of meet my need quickly, so I didn't bother with that part, it's a bit complicated after all. Since runtime performance is a concern, it's time to do this part ourselves