Evaluate yaml-rust2 - Githubissues

tweag / nickel

Better configuration for less

https://nickel-lang.org/

MIT License

2.43k stars 93 forks source link

Evaluate yaml-rust2 #2001

Open jneem opened 4 months ago

jneem commented 4 months ago

I had a go at adding positions to yaml, using the yaml-rust2 crate. It sort of works (without any error handling yet), but I also have some concerns:

the rust yaml ecosystem is kind of in flux right now. yaml-rust2 seems like the most active yaml library at the moment, but for how long?
only the low-level API supports positions, so I had to implement my own MarkedEventReceiver to construct the objects from the parser events. There's a marked_yaml crate, but it's pretty new and not much used yet.
the marks aren't always in quite the right position. See here; also, I haven't figured out how to easily and correctly find the position of a string unless it's in the "plain" style.

One alternative, as we already discussed, would be to wrap a mature C library.

yannham commented 4 months ago

Argh, we should have synced before. In fact I asked about this feature on an issue thread (https://github.com/Ethiraric/yaml-rust2/issues/27), and this was implemented in https://github.com/saphyr-rs/saphyr, which is from the same author but supposed to get more innovations (and potentially breaking changes) while yaml-rust2 would just get maintenance. So I think something very similar to what you implemented can already be done with saphyr, I just haven't had the time to look into it.

At the end of the corresponding PR, the author mentions that indeed the markers sent by their parser are off (see https://github.com/saphyr-rs/saphyr/pull/6#issuecomment-2208620304), and it's a bug of the parser.

We can either use saphyr already, if the imprecision is small, it's already better than what existed before. Or we can even contribute upstream to fix the parser; I suppose the saphyr maintainer now already knows about us and would be eager to merge this change, given they were eager to merge the marker change.

jneem commented 4 months ago

No worries, I would have checked first but I found myself with some unexpected free time today...

I will have a look at saphyr and see if I can figure out the position bug.

jneem commented 4 months ago

I had a quick look at libfyaml today. Their scanner already has support for recording the end of a token (which is basically the thing I'm trying to add to saphyr-parser). But their high-level API doesn't keep track of spans, so in addition to wrapping the C API we'd need to write the thing that consumes events and produces a tree (i.e. basically what's in this PR).