Clean up and modernize generate_packets.py
alienvalkyrie alienvalkyrie alienvalkyrie alienvalkyrie alienvalkyrie alienvalkyrie alienvalkyrie alienvalkyrie alienvalkyrie Note that also #44563 touches generate_packets.py. Please make sure that your patches do no conflict with that (it has a bit higher priority, as it fixes an actual build failure)
alienvalkyrie Reply To cazfi
Note that also #44563 touches generate_packets.py. Please make sure that your patches do no conflict with that (it has a bit higher priority, as it fixes an actual build failure)
I'm aware – in fact, that issue was what made me realize I should get my current work in a merge-ready state to avoid excess bitrot. I made sure all the patches of this recent batch apply cleanly on top of #44563.
alienvalkyrie alienvalkyrie btw. Does the order here match the order in which they should be applied? I may try to update (take out parts handled in other ticket) and rebase https://www.hostedredmine.com/issues/745593 on top of your patches soon.
alienvalkyrie Reply To cazfi
btw. Does the order here match the order in which they should be applied?
Not universally; they're grouped thematically, but the different topics are in the order they were added. It would probably be more useful to look at the order they were actually applied, e.g. the commit history on GitHub
alienvalkyrie A note on the kind of major things I'm planning on:
I'm planning on introducing a PacketsDefinition class, which many of the functions on the root module level will move into – at least all the ones that currently just take the packets list and return some code snippet. I'm also considering moving the script configuration stuff (most of the script arguments that aren't file paths) either into that class, or into its own config class, to cut down on global state.
Another thing I've been thinking about is to transform the code-producing functions into generators, the output of which is either "".join()ed (where necessary), or directly written into the target files. This will probably improve efficiency (since string concatenation is slow, and the resulting strings large), but more importantly, it will allow just yielding generated code, instead of lugging around parts of strings that need to be glued together and returned at the end.
The largest change I'm planning on is to introduce a hierarchy of field types. Currently, the ugliest part of the script, by far, is the mass of if-elses in various Field methods, determining how certain things should be done based on field type and array dimensions. My plan is to replace these if-elses with polymorphism – a number of separate classes for every kind of type that needs different treatment, with a common abstract base class (ABC) they all inherit from / implement. This will include a class for array types (that can have an arbitrary other field type as element type), and one (or more) for pseudo-types like string and memory which gobble up the first array dimension as their size to become a proper, useable type.
Given that this would break the in-progress patch for HRM#745593 beyond all recognition, I'm inclined to hold off on getting into this until that is done and merged; though I am still intending to finish this before S3_2 branching (ideally with time to spare; I'd be surprised if it doesn't take longer than anticipated).
alienvalkyrie Reply To alienvalkyrie
The largest change I'm planning on is to introduce a hierarchy of field types [ ... ] to replace these if-elses with polymorphism.
I've spent the past couple days prototyping this – for an idea of what the end result might look like, the prototype (or current state thereof) is on my github fork. (Unless you're reading this far enough in the future, when that branch will likely no longer exist.)
I won't be directly using that code – there are a few things I'll do differently, and some things I still want to try out before finalizing them. There's also a number of other changes I've made while prototyping, including significant improvements to the style/format of the generated code, which I'm planning to properly introduce to the main code base before any large-scale restructuring.
One thing I noticed is that there are remnants of things theoretically supported by the code, but not used by the network protocol (anymore), including 2D arrays, cm_parameter and city_map fields, and the no-packet flag – I haven't determined whether all of them actually still work. It might be worth considering whether some of these are worth removing.
alienvalkyrie alienvalkyrie alienvalkyrie alienvalkyrie alienvalkyrie alienvalkyrie alienvalkyrie alienvalkyrie Reply To alienvalkyrie
Reply To alienvalkyrie
The largest change I'm planning on is to introduce a hierarchy of field types [ ... ] to replace these if-elses with polymorphism.
There's also a number of other changes I've made while prototyping [ ... ] which I'm planning to properly introduce to the main code base before any large-scale restructuring.
Since the smaller changes – which were mostly about unifying the shape of code strings – are all done, I have now started work on implementing this for real (as opposed to a prototype), tracked in #45206.
One thing I noticed is that there are remnants of things theoretically supported by the code, but not used by the network protocol (anymore), including 2D arrays, cm_parameter and city_map fields, and the no-packet flag – I haven't determined whether all of them actually still work. It might be worth considering whether some of these are worth removing.
Note on these: no-packet (#44975) and city_map (#45012) have been removed; I'll keep generate_packets support for cm_parameter fields (since dataio still supports them), and I've determined that explicitly temporarily unsupporting the half-broken feature that is 2D arrays for the duration of the restructuring doesn't really provide any tangible benefit. In either case, it (along with a few other error-prone features) will fix itself for free along the way, so it will be usable in the future.
alienvalkyrie Given that the largest restructuring is done now, I figure it's time to look at what's left before I consider this "cleaned up":
(This is roughly the order in which I'll likely do these, based on importance and simplicity.)
alienvalkyrie alienvalkyrie alienvalkyrie Reply To alienvalkyrie
what's left before I consider this "cleaned up"
Since all of these are now done, I consider this cleanup effort finished now. There is always more to do, but the necessary groundwork has been laid to expand the script's features in the future, and I'm happy with the point we've reached.
This is a meta-ticket for various tasks relating to making common/generate_packets.py more clean and concise. This is mostly a refactoring effort, though some of these changes improve the generated code or make the script work in more contexts.
Apart from general code/style cleanup (which should happen at every step along the way), this includes:
(More to be added as they pop up.)