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

CachedClassProps and CachedProp really don't play well with any tooling. #86

Open
0Hughman0 opened this issue Sep 9, 2023 · 6 comments
Labels
bug Something isn't working documentation Improvements or additions to documentation

Comments

@0Hughman0
Copy link
Owner

0Hughman0 commented Sep 9, 2023

The accessors continue to cause chaos.

Sphinx autodoc doesn't manage to generate docstrings for any CachedClassProps, presumably because it just gets those attributes from the class, which get returned as the cached value.

This means things like TierBase.name_part_template are missing from the docs - they're important!

A solution may be found add some sort of preprocessor or whatever to Sphinx:

https://www.sphinx-doc.org/en/master/usage/extensions/autodoc.html#event-autodoc-process-docstring

The CachedClassProps can be fetch from the classes using the syntax (which bypasses the accessor protocol):

>>> TierBase.__dict__['name_part_template']
CachedClassProp

and it seems the wraps is passing on the correct docstring. So it should be possible to generate the docstring I think, provided we can find a way to get the right object to Sphinx.

@0Hughman0 0Hughman0 added bug Something isn't working documentation Improvements or additions to documentation labels Sep 9, 2023
@0Hughman0
Copy link
Owner Author

0Hughman0 commented Jul 9, 2024

Seems that the 'official' way to achieve class properties is now wrapping methods with @Property and @classmethod:

https://docs.python.org/3.9/library/functions.html#classmethod (python/cpython#63272)

And I believe this is also supported by Sphinx specifically:

https://www.sphinx-doc.org/en/master/usage/domains/python.html#directive-option-py-property-classmethod

But I'm not completely sure:

sphinx-doc/sphinx#9445

@0Hughman0
Copy link
Owner Author

0Hughman0 commented Jul 9, 2024

It may be possible to re-write cached_prop and cached_class_prop to be wrapped with property and classmethod in order to make these descriptors more officially supported and hopefully stop things constantly going wrong with them!

e.g.

class _ClsCached:

    def __init__(self, func: Callable[[Type[T]], V]) -> None:
        self.func = func
        self.val_set = False
        self.val = None

    def __call__(self, cls: Type[T]) -> V:
        if self.val is not None:
            return self.val
        
        self.val = self.func(cls)
        #self.val_set = True

        return self.val


def _cached_class_prop(wrapped: Callable[[Type[T]], V]):
    prop = property(_ClsCached(wrapped))
    cls_meth = classmethod(prop)
    return cls_meth

The only thing is this is all 3.9 and above, so would have to bump Python version dependency.

@0Hughman0
Copy link
Owner Author

Eugh, ok this was added in 3.9 and deprecated in 3.11:

python/cpython#110163

And mypy is responding accordingly:

python/mypy#11619

@0Hughman0 0Hughman0 changed the title Documentation misses CachedClassProps CachedClassProps and CachedProp really don't play well with any tooling. Jul 10, 2024
@0Hughman0
Copy link
Owner Author

Then goal of these methods is a way to set default values for certain attributes if they're not defined by subclasses...

Maybe there is another way?

@0Hughman0
Copy link
Owner Author

0Hughman0 commented Jul 10, 2024

For the class ones I can just do this!

Class A:

    @cached_class_prop
     def _default_a(cls):
         return 'a'
     
      a = _default_a

@0Hughman0
Copy link
Owner Author

See #131, I went with the option above.

I don't like stacking decorators, it's so confusing.

I think this implementation makes sense. If users subclass, then they sever the connection with the default setter. In theory we could probably simplify the implementation of cached_class_prop by removing all the type annotation stuff and just casting the type at the end. I think what I've done should technically work if the type checkers worked perfectly, so maybe one day they will. This solution is quite explicit so I'm happy with it.

Will close when merged.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug Something isn't working documentation Improvements or additions to documentation
Projects
None yet
Development

No branches or pull requests

1 participant