zmievsa / cached_classproperty

cached_property for class properties instead of instance properties
MIT License
7 stars 3 forks source link

Add cached_staticproperty #2

Closed zmievsa closed 11 months ago

zmievsa commented 1 year ago

Resolves #1

codecov[bot] commented 1 year ago

Welcome to Codecov :tada:

Once merged to your default branch, Codecov will compare your coverage reports and display the results in this comment.

Thanks for integrating Codecov - We've got you covered :open_umbrella:

Atry commented 1 year ago

The following test failed


def test__cached_classproperty__super():
    class Superclass:
        @cached_classproperty
        def my_cached_classproperty(cls):
            return ("Superclass",)
        @classmethod
        def my_classmethod(cls):
            return ("Superclass",)

    class Subclass1(Superclass):
        @cached_classproperty
        def my_cached_classproperty(cls):
            return (
                *super().my_cached_classproperty,
                "Subclass1",
            )
        @classmethod
        def my_classmethod(cls):
            return (
                *super().my_classmethod(),
                "Subclass1",
            )

    class Subclass2(Superclass):
        @cached_classproperty
        def my_cached_classproperty(cls):
            return (
                *super().my_cached_classproperty,
                "Subclass2",
            )
        @classmethod
        def my_classmethod(cls):
            return (
                *super().my_classmethod(),
                "Subclass2",
            )

    class Finalclass(Subclass1, Subclass2):
        @cached_classproperty
        def my_cached_classproperty(cls):
            return (
                *super().my_cached_classproperty,
                "Finalclass",
            )
        @classmethod
        def my_classmethod(cls):
            return (
                *super().my_classmethod(),
                "Finalclass",
            )

    assert Superclass.my_cached_classproperty == Superclass.my_classmethod()
    assert Subclass1.my_cached_classproperty == Subclass1.my_classmethod()
    assert Finalclass.my_cached_classproperty == Finalclass.my_classmethod()
    assert Subclass2.my_cached_classproperty == Subclass2.my_classmethod()
poetry run pytest
======================================================================= test session starts =======================================================================

platform win32 -- Python 3.11.0, pytest-7.2.2, pluggy-1.0.0
rootdir: C:\Users\atry\github\cached_classproperty
plugins: cov-4.0.0
collected 9 items                                                                                                                                                   

tests\test_classproperty.py ........F                                                                                                                        [100%]

============================================================================ FAILURES ============================================================================= 
________________________________________________________________ test__cached_classproperty__super ________________________________________________________________ 

    def test__cached_classproperty__super():
        class Superclass:
            @cached_classproperty
            def my_cached_classproperty(cls):
                return ("Superclass",)
            @classmethod
            def my_classmethod(cls):
                return ("Superclass",)

        class Subclass1(Superclass):
            @cached_classproperty
            def my_cached_classproperty(cls):
                return (
                    *super().my_cached_classproperty,
                    "Subclass1",
                )
            @classmethod
            def my_classmethod(cls):
                return (
                    *super().my_classmethod(),
                    "Subclass1",
                )

        class Subclass2(Superclass):
            @cached_classproperty
            def my_cached_classproperty(cls):
                return (
                    *super().my_cached_classproperty,
                    "Subclass2",
                )
            @classmethod
            def my_classmethod(cls):
                return (
                    *super().my_classmethod(),
                    "Subclass2",
                )

        class Finalclass(Subclass1, Subclass2):
            @cached_classproperty
            def my_cached_classproperty(cls):
                return (
                    *super().my_cached_classproperty,
                    "Finalclass",
                )
            @classmethod
            def my_classmethod(cls):
                return (
                    *super().my_classmethod(),
                    "Finalclass",
                )

        assert Superclass.my_cached_classproperty == Superclass.my_classmethod()
        assert Subclass1.my_cached_classproperty == Subclass1.my_classmethod()
>       assert Finalclass.my_cached_classproperty == Finalclass.my_classmethod()
E       AssertionError: assert ('Superclass', 'Finalclass') == ('Superclass'... 'Finalclass')
E         At index 1 diff: 'Finalclass' != 'Subclass2'
E         Right contains 2 more items, first extra item: 'Subclass1'
E         Use -v to get more diff

tests\test_classproperty.py:207: AssertionError
===================================================================== short test summary info ===================================================================== 
FAILED tests/test_classproperty.py::test__cached_classproperty__super - AssertionError: assert ('Superclass', 'Finalclass') == ('Superclass'... 'Finalclass')       
=================================================================== 1 failed, 8 passed in 0.11s ===================================================================

See https://github.com/atry/cached_classproperty/tree/add-cached-staticproperty

zmievsa commented 1 year ago

@Atry this looks like quite a normal behavior for cached properties, don't you think so?

Atry commented 12 months ago

I think @cached_classproperty should return the same value as @classproperty when the function does not have any side effect.

zmievsa commented 12 months ago

I am afraid it doesn't make sense as it breaks the "we run this thing once for every instance" rule because here classes are instances and this behavior would allow us to run it multiple times for a single instance if it was inherited.

I've taken a look at your test: only one of the asserts doesn't pass. All others do.

Also, an implementation would probably be pretty complex. What would you suggest?

Atry commented 12 months ago

I agree the "we run this thing once for every instance" rule. However, in test__cached_classproperty__super, Subclass2.my_cached_classproperty was not executed when cls is Finalclass.

I think the problem is that the cache keys of Subclass2.my_cached_classproperty and Superclass.my_cached_classproperty are the same, therefore they interfere each other

Atry commented 12 months ago

I wonder if using f"{id(self)}.{attrname}" or f"{id(owner)}.{attrname}" as the cache key would help.

zmievsa commented 12 months ago

Okay, your logic makes sense. Please, help me research how this specific situation is handled in other languages (Not the implementation but the behavior) and if it is indeed similar -- let's fix it and merge the new logic.

Atry commented 12 months ago

What else do you want to research other than the cache key uniqueness?

zmievsa commented 12 months ago

The fact that cached classproperties will re-execute for each parent class if called from a child class using super or any similar mechanism. If you believe that it is the case, then I'm ready to add the changes and merge them.

Atry commented 12 months ago

The fact that cached classproperties will re-execute for each parent class if called from a child class using super or any similar mechanism.

The behavior is expected to me. The only issue is the cache key uniqueness when the cached properties in subclass and superclass have the same name.

zmievsa commented 12 months ago

I guess let's think about the implementation, then. I'll try to tackle it this week. If you have time to play with it -- feel free to, I'll do my best to do reviews quickly.

Atry commented 11 months ago

I created https://github.com/Ovsyanka83/cached_classproperty/pull/3 to fix bugs that I reported in this PR