Incidencia #43927

Clean up and modernize generate_packets.py

Abrir Fecha: 2022-02-19 22:17 Última actualización: 2022-07-02 06:12

Informador:
Propietario:
Tipo:
Estado:
Open [Owner assigned]
Componente:
Hito:
Prioridad:
5 - Medium
Gravedad:
5 - Medium
Resolución:
Ninguno
Fichero:
Ninguno

Details

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 I expect some of these changes will 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.)

Ticket History (3/18 Histories)

2022-02-19 22:17 Updated by: alienvalkyrie
  • New Ticket "Clean up and modernize generate_packets.py" created
2022-02-19 22:27 Updated by: alienvalkyrie
  • Details Updated
2022-02-20 11:44 Updated by: alienvalkyrie
  • Hito Update from 3.1.0 to 3.2.0
  • Componente Update from General to Bootstrap
  • Details Updated
2022-02-21 03:19 Updated by: alienvalkyrie
  • Details Updated
2022-02-21 04:15 Updated by: alienvalkyrie
  • Details Updated
2022-02-22 23:09 Updated by: alienvalkyrie
  • Details Updated
2022-03-03 00:39 Updated by: alienvalkyrie
  • Details Updated
2022-05-13 03:34 Updated by: alienvalkyrie
  • Details Updated
2022-05-13 05:35 Updated by: alienvalkyrie
  • Details Updated
2022-05-13 09:01 Updated by: cazfi
Comentario

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)

2022-05-13 18:14 Updated by: alienvalkyrie
Comentario

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.

2022-05-14 01:13 Updated by: alienvalkyrie
  • Details Updated
2022-05-14 04:53 Updated by: alienvalkyrie
  • Details Updated
2022-06-07 03:14 Updated by: cazfi
Comentario

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.

2022-06-07 03:34 Updated by: alienvalkyrie
  • Details Updated
Comentario

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

2022-06-07 07:42 Updated by: alienvalkyrie
Comentario

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

2022-06-11 05:51 Updated by: alienvalkyrie
Comentario

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.

2022-07-02 06:12 Updated by: alienvalkyrie
  • Details Updated

Attachment File List

No attachments

Editar

You are not logged in. I you are not logged in, your comment will be treated as an anonymous post. » Entrar