Freeciv

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)

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.

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.

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

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).

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.

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.

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":

• Packet keys: correctly handling any number of key fields, and laying the foundation to support any type of key field in the future
• Cleaning up the (key-related) unfill stuff
• Turning the type selection if-else into a dictionary
• Parsing packet flags more straightforwardly (like Field flags)
• Adding docstrings to everything (and removing then-superfluous doc comments)
• Unhardcoding the input file (packets.def) path and making it an argument
• A reasonable general way to name indices for nested arrays of arbitrary depth
• A bunch of reusability stuff that's mainly intended for importing the module in custom generation scripts (e.g. for things like freeciv-web?):
  • Making it possible to replace the field type hierarchy with a custom set of subclasses
  • Moving all the configuration stuff out of global state

(This is roughly the order in which I'll likely do these, based on importance and simplicity.)