Skip to content

Refactor xml parser#473

Open
pv42 wants to merge 5 commits intomavlink:masterfrom
pv42:refactor_parser
Open

Refactor xml parser#473
pv42 wants to merge 5 commits intomavlink:masterfrom
pv42:refactor_parser

Conversation

@pv42
Copy link
Contributor

@pv42 pv42 commented Mar 11, 2026

The current xml parser is mostly in a single function with a lot of state, that has become increasingly hard to maintain.
This PR fixes this by rewriting the parser that generates the MavProfiles.

The (old) codegenerator and the new xml parser are in 2 seperate files. The old parser.rs still contains the rust code generation but is now named codegen.rs and the new parser is in parser.rs (the first commit does not rename the files yet so the diff works better).

Features of the new parser

  • better readebility, each xml tag has its own function
  • proper error handling for parser errors instead of panics/unwraps
  • more strict checks if xml is valid, most unexpected tags cause an error (could be changed)
  • fix tags not generating the note properly
  • add check if used enums exist (required adjusting of superseded test)
  • enum entries are now sorted by value instead of depth first include order
  • includes are resolved only after everything else is ready, this could be used for more effient parsing when the same xml file is used by multiple dialacts

The new parser still generates the same MavProfile as the old one, the only changes to the generated code are the fixed supersed bug and the ordering of enum entries.

Since this is a fairly large change I will add some test but unless I find any bugs this has all the functional changes.

Tests

  • add test for all new errors
  • add snapshot test for including
  • disable superseded test when mav2 extensions are enabled since it contains some and would fail since the expected snapshot does not have them
  • I manually compared the generated code for all dialects on 76ef372 with this and only differences are the 2 mentioned above

- splitt xml parser and code generator in 2 files
- compleatly rewrite xml parser for better readebility and robustness
- add proper error handling for parser errors
- fix superseed not generating the note properly
- add check if used enums exist
- enum entries are now sorted by value instead of depth first include order
- unknown tags cause an error
@pv42 pv42 force-pushed the refactor_parser branch from f31ba78 to 04f0eaf Compare March 11, 2026 15:26
@pv42 pv42 force-pushed the refactor_parser branch from 04f0eaf to f9d134f Compare March 11, 2026 15:28
@pv42 pv42 marked this pull request as ready for review March 13, 2026 15:08
@onur-ozkan
Copy link
Member

Impressive work. But unfortunately, I can't allocate the time to review such a massive change anytime soon, especially since it doesn't address an existing bug or provide a feature currently requested by users.

​The current code works fine and is easy to maintain for now. Replacing it with a from-scratch rewrite would require extensive testing and a very deep review which I cannot do and handle in the near future. Maybe others will do tho.

@pv42
Copy link
Contributor Author

pv42 commented Mar 13, 2026

But unfortunately, I can't allocate the time to review such a massive change anytime soon,

That is totally understandable you already doing a lot helping out the project.

There are bugs that this addresses but they are very minor (e.g. the description of superseded is not properly generated in some cases).

I personally find the current parser not very maintainable and would like to new things e.g. wip tags, that I would prefer to add to the new parser if it has a chance of getting merged.

It is not unfeasible to introduce the new parser gradually, so the review burden becomes less step (even tho the total is probably the same).
I could break it down into

  • new tests
  • new error handling (errors instead of panics)
  • parsing of single tag (e.g. a pr that only touches enums, etc.)
  • splitting/renaming files

If you think you would have the time for it (again I totally understand if you don't), I would get started on that.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants