WIP: Dataclass #31

HeMan · 2020-03-04T10:01:46Z

Would solve #23
Needs:

Documentation
Code cleanup
- Move datatypes to own file
- Short types mapping struct types "names" (what to do with boolean?)
- Remove the need for explicit inheritance from BinmapDataclass

HeMan · 2020-03-04T10:03:06Z

@philippeitis are you interrested in reviewing this?

philippeitis · 2020-03-04T16:52:14Z

I don't mind

philippeitis · 2020-03-04T21:53:53Z

Is there any reason you don't implement the data class like so:

    def __init_subclass__(cls, byteorder: str = ">"):
        """
        Subclass initiator.
        :param str byteorder: byteorder for binary data
        """
        cls._byteorder = byteorder
        dataclasses.dataclass(cls)
        type_hints = get_type_hints(cls)

        cls._formatstring = ""

        for field_ in dataclasses.fields(cls):
            _base, _type = datatypemapping[type_hints[field_.name]]
            if "constant" in field_.metadata:
                _base = ConstField
            elif "enum" in field_.metadata:
                _base = EnumField
            setattr(cls, field_.name, _base(name=field_.name))
            if type_hints[field_.name] is pad:
                _type = field_.default * _type
            if (
                type_hints[field_.name] is string
                or type_hints[field_.name] is pascalstring
                or type_hints[field_.name] is str
            ):
                _type = str(field_.metadata["length"]) + _type
            cls._formatstring += _type

Having to do @binmap.binmapdataclass every time you instantiate a class makes the code quite repetitive.

HeMan · 2020-03-05T10:07:58Z

Is there any reason you don't implement the data class like so:

    def __init_subclass__(cls, byteorder: str = ">"):
        """
        Subclass initiator.
        :param str byteorder: byteorder for binary data
        """
        cls._byteorder = byteorder
        dataclasses.dataclass(cls)
        type_hints = get_type_hints(cls)

        cls._formatstring = ""

        for field_ in dataclasses.fields(cls):
            _base, _type = datatypemapping[type_hints[field_.name]]
            if "constant" in field_.metadata:
                _base = ConstField
            elif "enum" in field_.metadata:
                _base = EnumField
            setattr(cls, field_.name, _base(name=field_.name))
            if type_hints[field_.name] is pad:
                _type = field_.default * _type
            if (
                type_hints[field_.name] is string
                or type_hints[field_.name] is pascalstring
                or type_hints[field_.name] is str
            ):
                _type = str(field_.metadata["length"]) + _type
            cls._formatstring += _type

Having to do @binmap.binmapdataclass every time you instantiate a class makes the code quite repetitive.

Yeah, it's either that way or removing the need to inherit from BinmapDataclass I think. Having the decorator could have a value since it clearly shows in the code that it is a dataclass.
Also, it will only be needed when defining the class, not when instantion, right?

Missed empty line

philippeitis · 2020-03-06T05:53:52Z

I don't think it matters if the code is a dataclass or not, since the code still behaves the same way. This is really just an implementation detail which the user should not have to think about - I'd stick with extending the class, since it's more informative about the methods and behaviours you'll inherit, as opposed to a decorator. And yes, it's only needed when declaring the class - my suggestion of moving the logic to the init_subclass works just fine, since we're directly modifying the class object.

- Add shortnames for types that matches tha names in struct.

philippeitis · 2020-03-04T16:54:03Z

binmap/__init__.py

-        if value in obj._enums[self.name]:
-            obj.__dict__[f"_{self.name}"] = value
+        datafieldsmap = {f.name: f for f in dataclasses.fields(obj)}
+        if type(value) is str:


if isinstance(value, str):

This allows subtypes which may be relevant here.

philippeitis · 2020-03-04T16:54:33Z

binmap/__init__.py

-            obj.__dict__[f"_{self.name}"] = value
+        datafieldsmap = {f.name: f for f in dataclasses.fields(obj)}
+        if type(value) is str:
+            datafieldsmap[self.name].metadata["enum"][value]


Why is this a dictionary access? I'd leave a comment explaining this if its supposed to be here.

philippeitis · 2020-03-04T16:57:47Z

binmap/__init__.py

+        if self.name in obj.__dict__:
+            raise AttributeError(f"{self.name} is a constant")
+        else:
+            obj.__dict__.update({self.name: value})


Why not set these values with obj.__dict__[self.name] = value?

philippeitis · 2020-03-04T16:58:44Z

binmap/__init__.py

+            obj.__dict__.update({self.name: value})
+
+
+char = NewType("char", int)


Typically, you want to put constants at the top of the file, with variable names in all caps to help identify them - eg: CHAR = NewType("char", int)

Hmm, you're right that constants usually are all caps, but since these are types I think it looks better and more consistent with other types to have them lower case. Also, already moved them to their own file in 41cffd4 and 1d2d109

philippeitis · 2020-03-04T16:59:56Z

binmap/__init__.py

+        setattr(cls, field_.name, _base(name=field_.name))
+        if type_hints[field_.name] is pad:
+            _type = field_.default * _type
+        if (


I would do type_hints[field_.name] in (string, pascalstring, str) here.

philippeitis · 2020-03-04T17:01:22Z

binmap/__init__.py

+class BinmapDataclass(ABC):
+    _byteorder = ""
+    _formatstring = ""
+    __binarydata: dataclasses.InitVar[bytes] = b""


Why do we have __binarydata and _binarydata?

I changed that in later (already pushed) changes.

philippeitis · 2020-03-04T17:05:08Z

binmap/__init__.py

        datafields = [
-            field for field in self._datafields if not field.startswith("_pad")
+            f.name for f in dataclasses.fields(self) if not (type_hints[f.name] is pad)


type_hints[f.name] is not pad

philippeitis · 2020-03-04T17:06:31Z

binmap/__init__.py

+        if _binarydata != b"":
+            self._unpacker(_binarydata)
+        # Kludgy hack to keep order
+        for f in dataclasses.fields(self):


Should we store our fields like so self._fields = dataclasses.fields(self)?

philippeitis · 2020-03-04T17:08:31Z

binmap/__init__.py

+    def __post_init__(self, _binarydata: bytes):
+        if _binarydata != b"":
+            self._unpacker(_binarydata)
+        # Kludgy hack to keep order


Should we maintain order through an internal class variable to allow the user to set the order themselves (for instance, _order)? This would also help avoid the self.__dict__.update(...) strangeness.

philippeitis · 2020-03-04T17:09:16Z

binmap/__init__.py

-        else:
-            self._binarydata = b""
+            if "constant" in f.metadata:
+                self.__dict__.update({f.name: f.default})


Again, this should be commented, but preferably, we should not be relying on this to maintain order.

philippeitis · 2020-03-09T03:30:15Z

Some of my comments may be off by a line or two - turns out the comments weren't submitted when I thought they were - hopefully they should still be close enough.

No need for decorator now.

HeMan · 2020-03-09T17:32:59Z

Addressed some of the comments and reworked some initializations.

HeMan · 2020-03-10T21:31:31Z

Merging it now, it could be easier to work with after that.

HeMan added 8 commits March 3, 2020 01:00

Initial dataclass chekin.

8e1afc7

First somewhat working version of dataclass.

77fced9

Move dataclass creation to binmapdataclass.

0cadc31

Make pading behave.

b1f5aef

Make constants work

d32ee75

Added all data types.

269ac31

Added some typing.

26d13fc

Add support for enums

7d7d27f

HeMan added 3 commits March 4, 2020 22:16

Updated documentation.

9368dc6

Make black and isort be friends.

7d60a73

Put types in own file.

41cffd4

HeMan marked this pull request as ready for review March 5, 2020 10:08

Make README render

bdb2df9

Missed empty line

HeMan added 3 commits March 6, 2020 22:29

Make binmapdataclass an implicit dataclass.

ca8a847

Make types an explicit import.

1d2d109

- Add shortnames for types that matches tha names in struct.

Removed unused attributes.

38b856a

HeMan force-pushed the dataclass branch from 2849dd2 to 38b856a Compare March 7, 2020 09:01

Remove ABC

ea7d5e0

philippeitis suggested changes Mar 9, 2020

View reviewed changes

Only need inheritance

761f587

No need for decorator now.

Simplified and moved some intialisations.

f138a53

HeMan merged commit fe96c69 into master Mar 10, 2020

This was referenced Mar 11, 2020

Add support for own validators #14

Closed

Support real enums. #22

Closed

HeMan deleted the dataclass branch March 12, 2020 20:31

HeMan restored the dataclass branch October 16, 2024 15:32

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

WIP: Dataclass #31

WIP: Dataclass #31

HeMan commented Mar 4, 2020 •

edited

Loading

HeMan commented Mar 4, 2020

philippeitis commented Mar 4, 2020

philippeitis commented Mar 4, 2020

HeMan commented Mar 5, 2020

philippeitis commented Mar 6, 2020

philippeitis Mar 4, 2020

philippeitis Mar 4, 2020

philippeitis Mar 4, 2020

philippeitis Mar 4, 2020

HeMan Mar 9, 2020

philippeitis Mar 4, 2020

philippeitis Mar 4, 2020

HeMan Mar 9, 2020

philippeitis Mar 4, 2020

philippeitis Mar 4, 2020

philippeitis Mar 4, 2020

philippeitis Mar 4, 2020

philippeitis commented Mar 9, 2020

HeMan commented Mar 9, 2020

HeMan commented Mar 10, 2020

		obj.__dict__.update({self.name: value})


		char = NewType("char", int)

WIP: Dataclass #31

WIP: Dataclass #31

Conversation

HeMan commented Mar 4, 2020 • edited Loading

HeMan commented Mar 4, 2020

philippeitis commented Mar 4, 2020

philippeitis commented Mar 4, 2020

HeMan commented Mar 5, 2020

philippeitis commented Mar 6, 2020

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

philippeitis commented Mar 9, 2020

HeMan commented Mar 9, 2020

HeMan commented Mar 10, 2020

HeMan commented Mar 4, 2020 •

edited

Loading