feat: error on duplicate keys

[wolfv]

lmk what you think – this would be nice for our use case, although it might be even better if the behaviour is configurable.

[kinnison]

I feel like (a) this would be better if it were optional and (b) the error should ideally tell us both locations.

[kinnison]

But fundamentally I like the idea of this

[wolfv]

Pushed changes. I assume mark contains the start of the mapping? I can also include it in the error if you think that's better.

[kinnison]

It's less mark and more that the key already in map which happens to "equal" the key we're trying to insert will have the span for the other location.

You could probably find that with a snippet like:

                                    let orig_key = map
                                        .keys()
                                        .find(|k| k.as_str() == key.as_str())
                                        .expect("Map claimed to contain key");

Then you could clone that key and put it into the error - enabling error messages along the lines of:

Duplicate key "foobar" found at 14:7 and 23:7

At least that way users know both locations when trying to hunt down why something was wrong.

That could be extra-helpful in cases where a file has nested mappings with repeated keys in the sub-mappings

[wolfv]

Hey @kinnison – I did push some changes that add both keys to the error. Just to note: clippy now complains about the error type being "large":

warning: the `Err`-variant returned from this function is very large
   --> marked-yaml/src/loader.rs:285:28
    |
28  |     DuplicateKey(MarkedScalarNode, MarkedScalarNode),
    |     ------------------------------------------------ the largest variant contains at least 176 bytes
...
285 |     fn finish(mut self) -> Result<Node, LoadError> {
    |                            ^^^^^^^^^^^^^^^^^^^^^^^
    |
    = help: try reducing the size of `loader::LoadError`, for example by boxing large elements or replacing it with `Box<loader::LoadError>`
    = help: for further information visit https://rust-lang.github.io/rust-clippy/master/index.html#result_large_err
    = note: `#[warn(clippy::result_large_err)]` on by default

What would be your preferred fix for that?

[wolfv]

Hey @kinnison would be happy to hear what you think about the changes :)

[kinnison]

Hi, sorry, I was travelling to, attending, and then travelling back from FOSDEM which took all of Fri->Mon so I've only just got to my email and notifications backlog. I like the Box approach, I think that's probably OK. Alternatively I suppose we could just include the marks, rather than the whole node, though I think the node is most useful. I'll spend some time tonight confirming I am happy and if so I'll merge and prepare a new release. Since this adds incompatible API I'll need to poke at it a bit to ensure I'm happy before I prepare something.

[wolfv]

Thanks, no worries :) Did you meet @orhun at the conference? Hope you had a nice time & maybe we'll meet there next year.

[kinnison]

I saw the ratatui talk but sadly I was unable to go up and say hi as I was then needing to run off to another meeting. Last night was a write-off but I have explicitly booked some time tonight to address this. Apologies for the slowness.

[orhun]

Oh nice, thanks for coming to my talk!

[wolfv]

Thanks for merging! Btw. feel free to change anything to your liking

[kinnison]

Thanks for merging!

You're welcome.

Btw. feel free to change anything to your liking

So far I like your work 😁

[wolfv]

Works like a charm: https://github.com/prefix-dev/rattler-build/pull/609/files#diff-2bd8fb5e1e9f9addad25613e8ec5ec2462b76eaf13cafa2e127e769a5b640a7c :)