open-dis-python icon indicating copy to clipboard operation
open-dis-python copied to clipboard

Refactoring, type annotations and fixes

Open ngjunsiang opened this issue 1 year ago • 4 comments

This pull request covers a number of changes:

Use DataQueryDatumSpecification and DatumSpecification in composition

This implements the change proposed in https://github.com/open-dis/open-dis-python/pull/49#issuecomment-1975744890

Before: https://github.com/open-dis/open-dis-python/blob/3a52c4fdb457c93c8223b903905761791c35654f/opendis/dis7.py#L6-L22

After: https://github.com/open-dis/open-dis-python/blob/b4ccb3dbe9fc71c7223a0e600bfff5ff29dd2510/opendis/dis7.py#L6-L24

Before: https://github.com/open-dis/open-dis-python/blob/3a52c4fdb457c93c8223b903905761791c35654f/opendis/dis7.py#L5042-L5067

After: https://github.com/open-dis/open-dis-python/blob/f62b7e33c9d5a1dfaf119c65143c7b8b384b120a/opendis/dis7.py#L4781-L4815

Any fixes to DataQueryDatumSpecification and DatumSpecification will not need to be copied to the classes that use it.

Style fixes

  • removed inheritance from object which is a Python2 requirement; Python 3 no longer requires it
  • edited most class docstrings to follow PEP257 conventions
  • removed redundant comments and whitespace
  • moved shared attributes (pduType, protocolFamily) from init to class attribute.

Bugfixes

Some parsing code I found to be non-compliant with IEEE 1278.1-2012 has been fixed. Also replaced a few null() calls with calls to the appropriate classes (addressing #14, but not yet completely fixed)

Add type annotations

This implements the change proposed in https://github.com/open-dis/open-dis-python/pull/49#issuecomment-1975744890

Type aliases

https://github.com/open-dis/open-dis-python/blob/76dedb414086dac7dd3e32c66e8751d4e6723469/opendis/dis7.py#L5-L19

The above type aliases are equivalent to the Python classes (int, float, bytes) assigned to them, and are used in type annotations to add more information about the required parameter type (enum, record (struct), signed or unsigned int, float) and size.

Type annotations have been added to most init parameters (some tricky ones are still unannotated), and it should be pretty clear from the annotations what is required now. In supported IDEs, these annotations can also be read and displayed in help text: image

Future work

This is groundwork for future pull requests aimed at organising and structuring the codebase, so as to make it easier to address the open issues.

In particular, the current code structure and flow (instantiating a single PDU class to wrap an input/output stream for serialization/parsing) makes it tricky to handle bitfields and sub-enumerations. I'm working on a parallel effort, a code generator for enumerations from SISO-REF-010 which can be used with this package to provide introspection (auto-lookup of enum values) and unpack bitfields.

ngjunsiang avatar Mar 11 '24 08:03 ngjunsiang

While this pull request is open, another quick question. I found this in the codebase:

https://github.com/open-dis/open-dis-python/blob/76dedb414086dac7dd3e32c66e8751d4e6723469/opendis/dis7.py#L273-L286

This class appears to be unused. I have left it in the codebase for now. If it is also legacy code (in the same category as the *Chunk classes), let me know and I'll push another commit to remove it.

ngjunsiang avatar Mar 11 '24 08:03 ngjunsiang

Thank-you @ngjunsiang I'm going to be unavailable for the next week but I'll provide some feedback when I'm back

leif81 avatar Mar 15 '24 16:03 leif81

quick ping @leif81 to see if there's anything else you need to see for this PR?

ngjunsiang avatar Apr 23 '24 03:04 ngjunsiang

Have started to review, I should be able to finish up in next couple days.

leif81 avatar Apr 23 '24 03:04 leif81