Fix missing messages when --message-format=json is deeply nested #6032
Conversation
|
(rust_highfive has picked a reviewer for you, use r? to override) |
|
Oh, this is awesome! Sorry, I meant to respond to you. I had played around with miniserde but I didn't get too far. I'll take a closer look tomorrow, but it does seem to fix the issue. I was wondering if you had an idea on how to easily make a test for it. I just made a 150 line macro, but I imagine there should be an easier way to exhibit the issue. |
|
I added a brief test and confirmed that it fails before this PR. |
This commit demonstrates using miniserde for the JSON messages emitted by machine_message::emit. Miniserde does not rely on recursion so this approach supports messages with any arbitrarily large amount of nesting. Pulling in a dependency on two different serialization libraries is not ideal, especially as this one is generally more verbose and annoying as a consequence of no recursion. We will need to discuss whether this tradeoff makes sense or whether the bug can be resolved a different way.
|
Nice! I ran it through some tests without problems. LGTM! If you're saying it is more verbose if there is an error, I think that should be rare, so I personally don't worry about that. |
|
Nice! In terms of maintenance though I'd probably prefer to not have two json parsers if we can as well as dual implementations of serialization. The underlying issue here seems somewhat niche to me in the sense that it doesn't necessarily justify the "hammer" of a dual serde implementation. |
|
Makes sense, I can see the downsides. |
This PR demonstrates using miniserde for the JSON messages emitted by
machine_message::emit. Miniserde does not rely on recursion so this approach supports messages with any arbitrarily large amount of nesting.Pulling in a dependency on two different serialization libraries is not ideal, especially as this one is generally more verbose and annoying as a consequence of no recursion. We will need to discuss whether this tradeoff makes sense or whether the bug can be resolved a different way.
I confirmed that this fixes #5992. @ehuss