Enhancements for introspection and debugging

[ngjunsiang]

Problem

While trying to test this package, I found that it was tedious to initialise classes from dis7 as the __init__() takes no arguments. That means PDUs and other objects must be instantiated first, then their attributes set one by one (or parsed from a stream).

I also saw the same complaint in #12 and thought this might be a useful enhancement.

Solution

A typical design for data classes is for the attribute values to be taken from the __init__() parameters, so that objects can be initialised in one line. This can be implemented without breaking any of the tests or existing code, by giving the parameters default values.

This PR shows the proposed change on three classes: DataQueryDatumSpecification, FixedDatum, and VariableDatum. I'd like to gauge interest (and any objections) in this feature first before I proceed to apply the same pattern to the remaining classes.

After PR

~/open-dis-python$ python
Python 3.10.11 (main, Apr  4 2023, 22:10:32) [GCC 12.2.0] on linux
Type "help", "copyright", "credits" or "license" for more information.
>>> from opendis.dis7 import DataQueryDatumSpecification
>>> dqds = DataQueryDatumSpecification(numberOfFixedDatums=1, numberOfVariableDatums=2)
>>> dqds.fixedDatumIDList
[]

[leif81]

The proposal makes sense to me @ngjunsiang .

I'll leave the PR open for a week to collect any other feedback . If there are no concerns raised as a comment here I'll go ahead and merge.

[ngjunsiang]

While this PR is still open, I have a couple of questions from going through the code in more detail as I add the init parameters:

open-dis-python/opendis/dis7.py

Lines 654 to 665 in 71c34b0

	class OneByteChunk( object ):
	"""8 bit piece of data"""
	def __init__(self):
	""" Initializer for OneByteChunk"""
	self.otherParameters = [ 0]
	""" one byte of arbitrary data"""
	def serialize(self, outputStream):
	"""serialize the class """
	for idx in range(0, 1):
	outputStream.write_byte( self.otherParameters[ idx ] );

I see a few *Chunk classes in dis7.py but don't see them being used anywhere. What are these used for?

open-dis-python/opendis/dis7.py

Lines 329 to 346 in 71c34b0

	def __init__(self):
	""" Initializer for AngleDeception"""
	self.recordType = 3501
	self.recordLength = 48
	self.padding = 0
	self.emitterNumber = 0
	self.beamNumber = 0
	self.stateIndicator = 0
	self.padding2 = 0
	self.azimuthOffset = 0
	self.azimuthWidth = 0
	self.azimuthPullRate = 0
	self.azimuthPullAcceleration = 0
	self.elevationOffset = 0
	self.elevationWidth = 0
	self.elevationPullRate = 0
	self.elevationPullAcceleration = 0
	self.padding3 = 0

I see in many classes that the padding is stored as an object attribute, although its value is always 0. This seems unnecessary when the padding can be added during serialization and ignored during parsing; is there a reason the padding value is kept?

[leif81]

@ngjunsiang the OneByteChunk was intended to be refactored out, but some occurences may have been missed. A pull request to remove any remaining is welcome.

[leif81]

@ngjunsiang About the padding, I don't know why it's like that, it's likely a relic of the old code generator. (Strike)Feel free to refactor that out too in a pull request.(/strike)

[leif81]

@ngjunsiang I remember now about the padding. It's there to be explicit and consistent with the spec. It's a field of particular size. Suggest we leave the padding as is.

[ngjunsiang]

__init__()

Okay, I've added __init__() parameters for almost all the Record and PDU classes. I have kept the padding attributes, though they are not included in the inits; that introduces the chance of a user setting a value to padding instead of another attribute.

Attribute renaming

The following attributes have undergone slight renaming for consistency (there were multiple slightly different names for them scattered throughout the code):

• numberOfFixedDatums
• numberOfVariableDatums
• fixedDatumIDs
• variableDatumIDs
• numberOfFixedDatumRecords
• numberOfVariableDatumRecords
• fixedDatumRecords
• variableDatumRecords

Removed *Chunk classes

Removed the *Chunk classes as requested.

[ngjunsiang]

Further proposed changes

I have a few other edits I'd like to make separately, after this merge, since they might change the implementation somewhat (even if I try to preserve the existing interface):

Use composition for DataQueryDatumSpecification and DatumSpecification

There are a number of PDUs that encapsulate datums. A quick count:

• Data Query PDU
• Action Request PDU
• Data PDU
• Set Data PDU
• Event Report PDU
• Comment PDU
• ... and their Reliable counterparts

These classes reimplement the attributes and serialization/parse methods, where composition could be used instead. The attributes can still be made available through the use of Python's property. This would reduce the amount of code to maintain and synchronise.

I have already spotted at least one bug where a DataQueryDatumSpecification is used instead of a DatumSpecification; this composition pattern would help make such occurrences easier to spot, and less work to fix.

Improved implementation for DataQueryDatumSpecification and DatumSpecification

These currently require the user to know upfront the number of datums, which is potentially fragile. The datum count is easily calculated using len() and this can be done automatically during serialization, or used for validation during parsing.

In parse() datums also should not be appended to the existing list of datums, since this will lead to an inaccurate count if reusing a PDU instance (is this usage pattern encouraged?) I plan to submit a bugfix after this merge.

Type annotations

I will also begin the work of adding type annotations, to enable the use of linters and static checkers for those who use them.