Using new, more restrictive field types for the binary header.

[abingham]

For example, we're using unsigned field types where appropriate to help avoid problems with physically impossible negative values.

The place to start with this review is in field_types.py where we define some new metaclasses to simplify the creation of various int-related field types.

Then look in binary_reel_header.py to see how the new metaclasses are applied.

[coveralls]

Coverage increased (+0.6%) to 70.736% when pulling 3cb9b2fbb28f0dc6bccd99079bd44c021cabd901 on abingham:improved-field-types into e03f1792fb6feb5947c98669467e9ae160a07b3b on sixty-north:master.

[rob-smallshire]

Can we aggregate an IntEnum into a field rather than inheriting?

On Thu, 4 Jan 2018 at 15:19, Austin Bingham notifications@github.com wrote:

@abingham commented on this pull request.

In segpy/field_types.py https://github.com/sixty-north/segpy/pull/67#discussion_r159662343:

• instance = super().new(cls, *args, **kwargs)
• if not (Int32.MINIMUM <= instance <= Int32.MAXIMUM):
• raise ValueError("{} value {!r} outside range {} to {}".format(cls.name, instance,
• cls.MINIMUM, cls.MAXIMUM))

• +class Int32(metaclass=IntFieldMeta,

• seg_y_type='int32'):
• pass

• +class UInt32(metaclass=IntFieldMeta,

• seg_y_type='uint32'):
• pass

• +class IntEnumFieldMeta(IntFieldMeta):

Hmmm. Creating a field-type that is also an IntEnum subclass is tricky, and might actually be impossible. Our fields have class-level attributes for things like min/max value, but Enums determine their constituents by looking for class-level attributes! If we try to add the class-level attributes after the enum-scanning machinery runs, we get an exception:

File "python3.6/enum.py", line 436, in _getmixins raise TypeError("Cannot extend enumerations")

— You are receiving this because your review was requested.

Reply to this email directly, view it on GitHub https://github.com/sixty-north/segpy/pull/67#discussion_r159662343, or mute the thread https://github.com/notifications/unsubscribe-auth/AA_uqbvbI02Vv_OQIz5GvUYHREArN7Clks5tHN4NgaJpZM4RSt4R .

[abingham]

Yeah, that should be pretty easy. And we can actually have the field instances be IntEnum instances. It feels shady, but it seems to work:

class IntEnumField2Meta(IntFieldMeta):
    def class_new(cls, *args, **kwargs):
        instance = super(cls, cls).__new__(cls, *args, **kwargs)

        # Here we transmute int values into IntEnums.  <======
        return cls.VALUES(instance)

    def __new__(cls, name, bases, namespace, enum, seg_y_type='int16', **kwargs):
        if any((value < LIMITS[seg_y_type].min)
               or (value > LIMITS[seg_y_type].max)
               for value in enum):
            raise ValueError(
                'Enum values must be within specified SEGY field type range.')

        # Here we aggregate the enum into the field <==========
        namespace['VALUES'] = enum

        return super().__new__(cls, name, bases, namespace, seg_y_type, **kwargs)

    def __init__(cls, name, bases, namespace, *args, **kwargs):
        super().__init__(name, bases, namespace)

FWIW, I've tried this out for the DataSampleFormatField, and it passes all of our tests.

[abingham]

@rob-smallshire I've updated the PR to reflect your review and some of the ideas we've been working with.

[coveralls]

Coverage increased (+1.6%) to 71.711% when pulling 416e439f489953885b70df6d63a039aa496b841f on abingham:improved-field-types into e03f1792fb6feb5947c98669467e9ae160a07b3b on sixty-north:master.

[coveralls]

Coverage increased (+1.6%) to 71.711% when pulling 416e439f489953885b70df6d63a039aa496b841f on abingham:improved-field-types into e03f1792fb6feb5947c98669467e9ae160a07b3b on sixty-north:master.