Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

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

Merged
merged 2 commits into from
Jul 20, 2023
Merged

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

merged 2 commits into from
Jul 20, 2023

Conversation

cdce8p
Copy link
Contributor

@cdce8p cdce8p commented Jul 20, 2023

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 python/cpython#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 python/cpython#26658.

The complete code I used to validate this change (tested with Python 3.8 - 3.12.0b4)
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
Copy link
Collaborator

puddly commented Jul 20, 2023

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
Copy link

codecov bot commented Jul 20, 2023

Codecov Report

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

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

❗ 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
@@            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 Coverage Δ
zigpy_znp/types/basic.py 100.00% <100.00%> (ø)

... and 1 file with indirect coverage changes

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

@puddly puddly merged commit e2fa419 into zigpy:dev Jul 20, 2023
@cdce8p cdce8p deleted the fix-cls-new branch July 20, 2023 23:57
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