Fix nested arrays (by reworking array handling) #465
Conversation
There was a problem hiding this comment.
Not a proper review, but verified it fixes the issue 👍
@matthiasbeyer (friendly ping) will hopefully have some time to spare to review / merge the bugfix.
matthiasbeyer
force-pushed
the
nested-array
branch
from
85e1319
to
a1b8369
Compare
|
Squashed the fixup commits for you and signed off with my own signoff because you didn't. I assume that you're alright with the signoff and its implications. Also, if you want, feel free to add your signoff to the commits as well! |
|
Meh. CI broke because a dependency of ours does no longer build with 1.66.0. I'll file a PR to update MSRV, after that is merged please rebase this (and signoff your commits while you're at it! 😆 ) please! |
I think by signoff you mean that you want my confirmation on the statements in the Developer Certificate of Origin , v1.1 presumably? I'm happy to confirm that all those statements apply to all of my contributions to You may want to copy |
Yes!
Ah, I thought that was the case. Good catch, I'll do this ASAP! |
|
I read in #483 (comment) that you're not intending a release any time soon. That's awkward for us. We need this bugfix for a feature we want to release at least a preview of fairly soon. Our current workaround is not really suitable. If you don't intend to release soon with this fix, we may need to publish a forked crate to crates.io, which I would rather avoid! |
|
Do I read this correctly that this PR is what you'd need for your project? Is there a chance that we can backport the fix to the I just did a quick test run. All the commits from this PR seem to apply cleanly to the release branch and tests run green. |
Have all the various versions of sequences (arrays and various forms of tuple) all go via ser::SerializeSeq. This reduces some duplication. And, we're about to change the implementation. Signed-off-by: Ian Jackson <[email protected]>
We're going to want to do something more complicated. In particular, to handle nested arrays properly, we need to do some work at the start and end of each array. The `new` and (inherent) `end` methods of this newtype is where that work will be done. Signed-off-by: Ian Jackson <[email protected]>
Change the representation of our "current location". Previously it was a list of increasingly-long full paths, but excepting the putative final array index. Now we just record the individual elements. and assemble the whole path just before calling .set(). This saves a little memory on the whole since the entries in keys are now a bit shorter. It is much less confusing. (There are perhaps still further opportunities to use Rust's type system to better advantage to eliminuate opportunities for bugs.) Arrays that appear other than at the top level are now handled correctly. This includes nested arrays, and arrays containing structs, etc. Signed-off-by: Ian Jackson <[email protected]>
Signed-off-by: Ian Jackson <[email protected]>
|
Rebased, thanks. Yes, a backport of this to the release branch would be perfect for us. I can make an MR for that right away if you like. |
|
Not sure I understand why the MSRV CI test is still failing. Did I rebase onto the wrong thing? |
Please do! ❤️ But after this PR went in and with |
You did not rebase on latest master... |
ijackson
force-pushed
the
nested-array
branch
from
a1b8369
to
aa63d2d
Compare
|
I think my push didn't take or something? Done now I think. Sorry. |
Nah don't worry! 👍 |
Fixes #464.