Skip to content

Conversation

@isHarryh
Copy link
Contributor

@isHarryh isHarryh commented Jan 30, 2025

Summary

This PR mainly fixes some type hints in EndianBinaryReader and EndianBinaryWriter.

By the way, some minor code issues are fixed, including:

  1. Legacy code lines (e.g. legacy pass statement) are removed.
  2. Some imports and isinstance-checks are optimized.
  3. Some potential errors (e.g. int conversion) are fixed.

For more details, please refer to the commits.

K0lb3
K0lb3 previously approved these changes Feb 3, 2025
Copy link
Owner

@K0lb3 K0lb3 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks a lot 👍

@K0lb3
Copy link
Owner

K0lb3 commented Feb 3, 2025

What is the point of builtins.bytes though?
Isn't it the same as the normal bytes?

@isHarryh
Copy link
Contributor Author

isHarryh commented Feb 4, 2025

What is the point of builtins.bytes though? Isn't it the same as the normal bytes?

Here is a minimal example shows why bytes without builtins doesn't work:

class EndianBinaryReader:
    endian: str
    ...

    def __new__(cls, ...):
        ...

    def __init__(self, ...):
        ...

    @property
    def bytes(self):
        # implemented by Streamable and Memoryview versions
        return b""

    ...

    def read_bytes(self, num) -> builtins.bytes:
        return self.read(num)

    ...

In the code above, bytes was first declared as a property name. This will overwrite the original bytes (the type name).

So, if the type hint still uses bytes, the linter will recognize it as the property name bytes, which will cause some type hint error.

image

To avoid this problem, it's recommended to use builtins.bytes in the type hint.

@isHarryh
Copy link
Contributor Author

isHarryh commented Feb 4, 2025

I caught some other type hints issues in EndianBinaryReader (e.g. struct.unpack returns tuple instead of list). I have force-pushed these changes.


By the way, I want to ask a small question:

class EndianBinaryReader_Memoryview(EndianBinaryReader):
    __slots__ = ("view", "_endian", "BaseOffset", "Position", "Length")
    view: memoryview

    def __init__(self, view, endian: str = ">", offset: int = 0):
        self._endian = ""
        super().__init__(view, endian=endian, offset=offset)
        self.view = memoryview(view)
        self.Length = len(view)

Why you use self._endian = ""? Is this intentional? Why not self._endian = endian?

@K0lb3
Copy link
Owner

K0lb3 commented Feb 4, 2025

In the code above, bytes was first declared as a property name. This will overwrite the original bytes (the type name).
I see, makes sense. So far it worked fine for me without issues, but I'm not against using this fix if it's causing issues for you and potentially others as well.

Why you use self._endian = ""? Is this intentional? Why not self._endian = endian?
@endian is a property that sets "optimized" functions. As it checks for a difference in endian at that point, the initial _endian value has to be set to an unaccepted value, so that the .endian property call in __init__ can detect an endian change, thereby setting the correct endianed reader functions.

It's certainly not a great solution and could be done better.
Assigning endianed functions yields a sizeable performance increase though, which is why I'm sticking with it so far.
For the next minor update I already made a small rewrite for this, which assigns functions instead of overwriting the __class__, as the later isn't allowed in the latest python versions anymore.

@K0lb3 K0lb3 merged commit e8ca4cb into K0lb3:master Feb 4, 2025
3 checks passed
@isHarryh isHarryh deleted the patch branch February 7, 2025 03:43
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants