-
Notifications
You must be signed in to change notification settings - Fork 912
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
msggen: fix walk through nested json schemas #7218
msggen: fix walk through nested json schemas #7218
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Looks good, but I'd like the if
statement to be clarified a bit, as the precedence of the conditions is not clear, and left-to-right order is even a tautology.
7c6a32e
to
d440033
Compare
need #7217 merged first I had to merge some arrays so it got more complicated :/ |
1227d9a
to
4da2e18
Compare
I WILL BRING ORDER TO THIS AAAAHHH I'm honestly not sure why i lost the deterministic ordering of the fields of all the generated code but now its |
4da2e18
to
0be4b26
Compare
|
This comment was marked as resolved.
This comment was marked as resolved.
This comment was marked as resolved.
This comment was marked as resolved.
This comment was marked as resolved.
This comment was marked as resolved.
0be4b26
to
e54288a
Compare
We derived the i32 from rust enums implicitly by their order in the enum. I added explicit numbers in #7217 so the sorting doesn't mess anything up. Btw if we ever delete an "open" enum field in *.proto with |
We might want to keep dummy placeholders in there once we do. But we'll cross that bridge once we get to it. I don't fully understand why sorting is required here, does it have to do with the iteration through nested fields? Otherwise it feels strange to have enums not be in numeric order, but no real issue, I'm just curious :-) |
Yes when merging the arrays that were previously not collected we would get enums in node.proto that were out of order. And protoc wants them to start at 0. The ordering of enums in the rust files could be removed eagain after the explicit enum values were added. I don't remember exactly why i sorted those to be honest. Want me to check that out? |
e54288a
to
fa6c8f2
Compare
rebased and rust model enums sorted by number not name |
Changelog-None
fa6c8f2
to
c93d955
Compare
Rebased on top of |
i have a bunch more coming but i wait until this and #7230 is merged |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
ACK c93d955
I finally found (what i suspected in the first place) the bug in
msggen
where it wasn't walking through theif
's correctly (you may have noticed the weird "x doesn't have a type" logs.Fixes: #7185 #5345 #5354
Replaces: #7210