✨ xmlgen: Maintain definition order

[z33ky]

I found it a bit odd that zbus-xmlgen reorders the fields and did something about it. By default it does keep the previous behavior, but it can now keep the file ordering with a --file-order flag to zbus-xmlgen, or if used by code then the CodeGenerator::with_sorted() method.

I don't believe the changes have a noticeable impact on performance. This is a breaking change though, considering the public interface of zbus_xml::Interface changed in a non-backwards compatible way.

I've also considered renaming the Interface::methods()-like methods to iter_methods() to make in clear from the name that an iterator is returned. Let me know if this is something you'd prefer - if you're interested in taking this change at all.

---

The --file-order flag, or for the CodeGenerator the sorted flag, toggles whether to sort elements by type and alphabetically, or whether to keep the order as defined by the XML specification.

[codspeed-hq]

Merging this PR will not alter performance

✅ 24 untouched benchmarks

---

_{Comparing z33ky:xmlgen-file-order (c184911) with main (258519e)}

[zeenix]

• main branch is now an API break so you don't need to care about breaking API or behaviour much, especially if it doesn't mean users having to do a lot to port to the next version.
• I'd prefer this to be just the default behaviour unless there's a compelling reason to allow both.
• Didn't really get a chance to look deeper into the changes yet but are you sure what's said in the title and description is all the code is doing and if the only commit here is atomic?

[z33ky]

I'd be fine if this was the default, or even the only behavior. It sounds like you may have meant the latter? This would remove a bit, though not a lot, of the code. Mostly the split done here would not be necessary, and of course removing the CodeGenerator::sorted variable and --file-order flag, but yeah that's not a lot of code. The tests being split into sorted and unsorted could be slimmed back down.

I separated the changes to a degree which I find sensible, albeit the facilitating refactors may be a little shaky by themselves, though I justify the change in the commit message to state that they facilitate a following change.

[zeenix]

I'd be fine if this was the default, or even the only behavior. It sounds like you may have meant the latter?

Yes. A good UX works as expected by default and an option is only added if there is a compelling enough use case.

I think this could be a much simpler change as we just need to ensure that we store everything in the order it is parsed and then iterate in order (which is the default for Vec and BTreeMap).

[z33ky]

I'd be fine if this was the default, or even the only behavior. It sounds like you may have meant the latter?

Yes. A good UX works as expected by default and an option is only added if there is a compelling enough use case.

That may very well depend on who you ask, but removing the former behavior outright is fine by me.

Do you want to remove 9d7f0ea then and have these inline in the match arms?

I think this could be a much simpler change as we just need to ensure that we store everything in the order it is parsed and then iterate in order (which is the default for Vec and BTreeMap).

That would be Interface::children, a Vec<Child> where Child is a new enum that can be any of the child-element types (Method, Property, ...).

The only other thing I can think of right now to simplify would be to remove the Interface::method()-type methods, that filter the children list by type, providing an API similar to before. But I don't think that really simplifies it that much. I don't need the methods myself, I am just using the zbus-genxml tool, but I can see them being convenient. Removing these would cause some churn in the tests, which use these, but it shouldn't be that much to adapt.

If there's something more concrete you have in mind or want to talk over I'm up for discussion.

[zeenix]

I'd be fine if this was the default, or even the only behavior. It sounds like you may have meant the latter?

Yes. A good UX works as expected by default and an option is only added if there is a compelling enough use case.

That may very well depend on who you ask, but removing the former behavior outright is fine by me.

That's why you don't ask but rather choose the best behaviour that works for most folks and if there is anyone needing the old behaviour, they can ask for an option (I doubt it though). The only issue with the new behaviour would be reordering of the generated code but that's fine, especially given that it'll be a semver bump.

If there's something more concrete you have in mind or want to talk over I'm up for discussion.

I'm having trouble following because this PR add a major refactor/cleanup along with the main change (I'd suggest splitting the PR to make this easier to discuss and review) but what I want is what I wrote above: Ensure all nodes are parsed into their containers in the order they're parsed from the file, using container types that keep the order and then iterations etc become ordered naturally and there is no need for special API.

[z33ky]

I don't know about major refactor. The only changes I do is in order to facilitate implementation of the feature itself.

IMO the diffs are reasonably sized.

d1c53fdf (HEAD -> xmlgen-file-order, origin/xmlgen-file-order) ✨ xmlgen: Add support for --file-order CLI flag
 zbus_xmlgen/src/cli.rs  | 5 +++++
 zbus_xmlgen/src/main.rs | 3 ++-
 2 files changed, 7 insertions(+), 1 deletion(-)
db5c1112 ✨ xmlgen: Add support for CodeGenerator::with_sorted()
 zbus_xml/src/lib.rs    |  5 +++++
 zbus_xmlgen/src/lib.rs | 65 ++++++++++++++++++++++++++++++++++++++++++++++++++---------------
 2 files changed, 55 insertions(+), 15 deletions(-)
501b0038 ♻️ xml: Change Interface to store a single list of child elements
 zbus_xml/src/lib.rs | 187 +++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++----------------------------------
 zbus_xml/src/xml.rs |  28 ++++++++++------------------
 2 files changed, 163 insertions(+), 52 deletions(-)
9d7f0ea5 ♻️ xmlgen: Split some code from CodeGenerator::write_interface() into dedicated functions
 zbus_xmlgen/src/lib.rs | 133 ++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++-----------------------------------------------------
 1 file changed, 80 insertions(+), 53 deletions(-)
e6f8b179 ♻️ xml: Return Iterators to access Interface elements
 zbus_xml/src/lib.rs    | 20 ++++++++++----------
 zbus_xmlgen/src/lib.rs | 15 +++++++--------
 2 files changed, 17 insertions(+), 18 deletions(-)

(Excluding tests.)

You can visit and comment on each commit in the PR individually using the GitHub UI.

Do you want 5 PRs then or one for the three refactorings and one for the two feature implementation commits?

[zeenix]

Do you want 5 PRs then or one for the three refactorings and one for the two feature implementation commits?

No, I meant more like split the refactor but don't worry about it. I went through the commits and they seem to make sense. Just one thing I pointed out, rebasing on the latest main and satisfying the commitling job, and we should be good to merge.

I hope you're fine with AI review as I like to do some rounds of that becuase they can catch things I would not.

[z33ky]

There are merge conflicts, I'll probably check it out over the weekend.

When I've pushed the new version feel free to run it through AI review. I personally object to LLM usage, but this is not the platform to have that discussion. I also do not deny their usefulness. So feel free.

[z33ky]

Alight, wasn't that difficult to fix ;)

I took out 9d7f0ea. Not sure if you liked it regardless, but it's not necessary if there's only one call site per function, so now it's inline as before.

[zeenix]

Apart from that one nitpick, 2 more regarding the commit messages:

• The last commit is not exactly a breaking change. We never guaranteed any specific order for generated code and there is no reason people should expect it untill we do so 💥 isn't the best prefix for it.
• Please don't break lines just to start a new sentence. I know it's pretty trendy these days but it's grammatically incorrect and it's benefits are completely subjective.

[z33ky]

💥 isn't the best prefix for it

I was actually looking if that'd fix the semver-checks failure XD

Please don't break lines just to start a new sentence. I know it's pretty trendy these days but it's grammatically incorrect and it's benefits are completely subjective.

I'm personally not a fan either, so I'm happy to change it. It is a prevalent style though indeed.
I also prefer soft limits rather than hard limits on line lengths for a similar reason.

I read that wrong. I assume you mean in the commit message.

zeenix changed the title ✨ xmlgen: Add support for --file-order flag ✨ xmlgen: Add --file-order flag 23 minutes ago

Do note that there is no longer a --file-order flag.

[zeenix]

💥 isn't the best prefix for it

I was actually looking if that'd fix the semver-checks failure XD

Ah, the semver-check is very much expected to fail right now. It'll be fixed when 6.0 is out.

Please don't break lines just to start a new sentence. I know it's pretty trendy these days but it's grammatically incorrect and it's benefits are completely subjective.

I'm personally not a fan either, so I'm happy to change it. It is a prevalent style though indeed. I also prefer soft limits rather than hard limits on line lengths for a similar reason.

I think you misunderstood. The line limit is 74 chars, as it says in the docs pointed out from our contributing guide. I was talking about breaking the line just to start a new sentence. i-e please only break the line to either start a new paragraph (and then you need an empy line in between the paragraphs) or for complying with the character limit for each line.

zeenix changed the title ✨ xmlgen: Add support for --file-order flag ✨ xmlgen: Add --file-order flag 23 minutes ago

Do note that there is no longer a --file-order flag.

Right. Please update the PR title/description accordingly. :)

[zeenix]

We're getting close to a merge, I think.

[zeenix]

sorry, found one potentially big issue.

[zeenix]

nit: can be simpler if we just name interface_children to children.

[zeenix]

🤖 ChatGPT GPT-6 Astra Pro on behalf of zeenix: Renaming the local also requires qualifying the children(...) parser-helper call immediately below as self::children(...); otherwise the new local shadows the function.

[zeenix]

These 2 are not really children so a bit strange to have them here. Besides we don't write neither of them so what's the usecase of having them here? Having them aslo means that our check of "if children present" becomes more inefficient than it would otherwise be.

[zeenix]

🤖 ChatGPT GPT-6 Astra Pro on behalf of zeenix: [P2] This also introduces a Serde serialization failure. #[serde(skip)] on an enum variant returns an error when that variant is serialized; it does not omit the item from the surrounding Vec. Consequently, parsing the existing telepathy_type_definitions test input and calling serde_json::to_string(&node) now fails at Child::TelepathyTypeDef. Previously, the skipped Interface::telepathy_types field was simply omitted.

Keeping these definitions outside the serializable children, as suggested above, avoids this regression; otherwise the serializer needs to filter them explicitly. Please cover this with a direct Serde test—Node::to_writer takes a separate path and will not catch it.

[zeenix]

🤖 ChatGPT GPT-6 Astra Pro on behalf of zeenix: Reviewed all four commits at c184911. The code-generation ordering change looks consistent with the agreed behavior, but there are two P2 regressions in the retained Serde implementations: interface members are silently lost when deserializing standard introspection XML, and interface-level Telepathy definitions make serialization fail. The first is detailed inline; the second is added to the existing Telepathy-children thread.

Commit hygiene: 2dc5a55 still introduces the public Child API and changes Node::to_writer ordering under a ♻️ refactor title. Please make Child public in the accessor commit (0ff6516) and accurately describe/categorize the observable XML-writer behavior change. The final commit changes code generation, not parsing as its message currently says.

Current CI is green. This was a source review; no local tests were run because this environment has no Rust toolchain.

[zeenix]

🤖 ChatGPT GPT-6 Astra Pro on behalf of zeenix: [P2] Preserve the Serde mapping for interface members. The retained Deserialize implementation now recognizes a children field, not the existing <method>, <signal>, <property>, or <annotation> elements. Consequently, deserializing <node><interface name="org.example.Test"><method name="Ping"/></interface></node> with quick_xml::de::from_str::<Node<'_>> ignores method and defaults children to an empty vector instead of producing the Ping method.

Please map this collection to the XML child elements (e.g. $value with lowercase variant tag names, or an equivalent custom implementation) and add a direct Serde regression test. The existing test named serde only exercises Node::from_reader/to_writer, which now use the hand-written parser/writer and do not validate these derives.