zigpy / zigpy-znp

TI CC2531, CC13x2, CC26x2 radio support for Zigpy and ZHA
GNU General Public License v3.0
144 stars 40 forks source link

Don't reassign cls.__new__ for Python 3.10+ #219

Closed cdce8p closed 11 months ago

cdce8p commented 11 months ago

I've started testing Home Assistant with Python 3.12. One common error is

TypeError: int() can't convert non-string with explicit base

After some investigation I tracked it back to zigpy-znp and indeed, running the test suite results in this error

ImportError while loading conftest '/usr/src/tests/conftest.py'.
tests/conftest.py:27: in <module>
    import zigpy_znp.config as conf
zigpy_znp/config.py:23: in <module>
    from zigpy_znp.commands.util import LEDMode
zigpy_znp/commands/__init__.py:1: in <module>
    from .af import AF
zigpy_znp/commands/af.py:28: in <module>
    class AF(t.CommandsBase, subsystem=t.Subsystem.AF):
zigpy_znp/types/commands.py:192: in __new__
    .with_id(definition.command_id)
zigpy_znp/types/commands.py:122: in with_id
    return type(self)(self & 0x00FF | (value & 0xFF) << 8)
zigpy_znp/types/commands.py:98: in __new__
    instance = super().__new__(cls, value)
zigpy_znp/types/basic.py:48: in __new__
    instance = super().__new__(cls, *args, **kwargs)
E   TypeError: int() can't convert non-string with explicit base

It can be reproduced by executing just

from zigpy_znp.types.commands import CommandHeader
CommandHeader().with_id(4)

It is likely related to a change in cpython regarding reassignments of cls.__new__ and super(), see https://github.com/python/cpython/issues/106917.

As it turns out though, the reassignment isn't actually needed anymore. The tests added in 72402da pass for Python 3.10.0+ without it. Likely a side effect of https://github.com/python/cpython/pull/26658.

The complete code I used to validate this change (tested with Python 3.8 - 3.12.0b4) ```py import enum import sys class FixedIntType(int): _signed: bool _size: bool def __new__(cls, *args, **kwargs): if getattr(cls, "_signed", None) is None or getattr(cls, "_size", None) is None: raise TypeError(f"{cls} is abstract and cannot be created") print(f"=> {cls}, {args=}") instance = super().__new__(cls, *args, **kwargs) return instance def __init_subclass__(cls, signed=None, size=None) -> None: super().__init_subclass__() if signed is not None: cls._signed = signed if size is not None: cls._size = size print(cls.__dict__) if sys.version_info < (3, 10): if "__new__" not in cls.__dict__: cls.__new__ = cls.__new__ print(cls.__dict__) class uint_t(FixedIntType, signed=False): pass class uint8_t(uint_t, size=1): pass class uint16_t(uint_t, size=2): pass class CustomInt(uint16_t): def __new__(cls, value: int = 0): instance = super().__new__(cls, value) return instance def with_id(self, value: int): return type(self)(self & 0x00FF | (value & 0xFF) << 8) CustomInt().with_id(4) class enum_uint(uint8_t, enum.Enum): pass class AddrMode(enum_uint): """Address mode.""" NOT_PRESENT = 0x00 Group = 0x01 NWK = 0x02 IEEE = 0x03 Broadcast = 0x0F ```
puddly commented 11 months ago

Thanks for the PR! Could you run it through the pre-commit hooks and push a commit to fix the lint errors?

pip install pre-commit
pre-commit install
pre-commit run --files zigpy_znp/types/basic.py

You can also make a whitespace-only change and the hook will run automatically upon commit to correct things.

codecov[bot] commented 11 months ago

Codecov Report

Patch coverage: 100.00% and project coverage change: +0.05 :tada:

Comparison is base (686cb6c) 98.46% compared to head (fefb3a0) 98.52%.

:exclamation: Current head fefb3a0 differs from pull request most recent head 1f160f3. Consider uploading reports for the commit 1f160f3 to get more accurate results

Additional details and impacted files ```diff @@ Coverage Diff @@ ## dev #219 +/- ## ========================================== + Coverage 98.46% 98.52% +0.05% ========================================== Files 43 43 Lines 3785 3787 +2 ========================================== + Hits 3727 3731 +4 + Misses 58 56 -2 ``` | [Impacted Files](https://app.codecov.io/gh/zigpy/zigpy-znp/pull/219?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=zigpy) | Coverage Δ | | |---|---|---| | [zigpy\_znp/types/basic.py](https://app.codecov.io/gh/zigpy/zigpy-znp/pull/219?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=zigpy#diff-emlncHlfem5wL3R5cGVzL2Jhc2ljLnB5) | `100.00% <100.00%> (ø)` | | ... and [1 file with indirect coverage changes](https://app.codecov.io/gh/zigpy/zigpy-znp/pull/219/indirect-changes?src=pr&el=tree-more&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=zigpy)

:umbrella: View full report in Codecov by Sentry.
:loudspeaker: Do you have feedback about the report comment? Let us know in this issue.