Fixed #29478 -- Added support for mangled names to cached_property. - #10033

I think this should throw a warning on < PY36 where re.startswith('__') and not re.endswith('__')

Not sure about the best way to do that though

I think a plain warning would be fine.

People like to copy this file into their non Django related files. I'd like to avoid importing Django here for that reason

Could be a plain warnings.warn

Also not sure that the 'people like to copy this file' argument holds for Django's codebase. There's pip install cached-property for an implementation of this outside of Django.

actually this is redundant because PY35 is no longer supported:

eg I can't do: if django.utils.version.PY35 and name is None and func.__name__.startwith('__') and not func.__name__.startwith('__') anyway

Wrap this into previous paragraph so we only have one explanation of name?

On Python <3.6, the ``name`` argument should be passed to make a cached property from another method without a name collision, e.g. ``url = cached_property(get_absolute_url, name='url')``.

done

I think a plain warning would be fine.

I think we'd normally call this check_cached_property or similar

Also I'd prefer a longer class name than A whilst we're refactoring these tests

We typically follow the camel casing of unittest assertions, e.g. assertCachedPropertyWorks. Try to split out test refactoring in a separate PR to ease review.

currently the refactorings are in separate commits, here's the copy/paste version: 0870388

@timgraham I've also included a call to with self.subTest

Can you just do name.startswith('__') and not name.endswith('__')? Simpler is better

PY35

ooops

I'm not sure what Adam was thinking of with regards to the release note, but we don't usually document changes in django.utils to be "minor features".

Do you know where it should go?

Most likely it doesn't need a release unless there's some backwards incompatible change.

I've seen a lot of use of @cached_property so I thought it would count, but yeah let's not worry about it.

It should be documented when 3.5 is dropped and name is deprecated

removed

@graingert cls is passed here.

yeah but it's the wrong cls

I think it's the right class:

In [38]: class desc:
    ...:     def __get__(self, instance, cls):
    ...:         if instance is None:
    ...:             return self
    ...:         return instance.__dict__['_%s__mangled' % cls.__name__]

In [39]: class A:
    ...:     d = desc()
    ...: 
    ...:     def __init__(self):
    ...:         self.__mangled = 42
    ...: 

In [40]: A().d
Out[40]: 42

Try with a subclass of A

You might want to add tests for the subclass case @graingert to make it clear.

@graingert Probably it doesn't worth it, but still doable :-)

In [85]: class desc:
    ...:     def __init__(self, f):
    ...:         self.name = f.__name__
    ...: 
    ...:     def __get__(self, instance, cls):
    ...:         if instance is None:
    ...:             return self
    ...: 
    ...:         for cls in cls.__mro__:
    ...:             name = '_%s%s' % (cls.__name__, self.name)
    ...:             if cls.__dict__.get(name) is self:
    ...:                 return name
    ...: 
    ...: class A:
    ...:     @desc
    ...:     def __x(self):
    ...:         pass
    ...: 
    ...: 
    ...: class B(A):
    ...:     pass
    ...: 

In [86]: a = A()
    ...: a._A__x
    ...: 
Out[86]: '_A__x'

In [87]: b = B()
    ...: b._A__x
    ...: 
Out[87]: '_A__x'

You'll want to branch off greater or equal to Python 3.6 here.

that's how this works already

You'll want to branch of lesser than Python 3.6 here.

that's how this works already

Python < 3.6.

done

You might want to add tests for the subclass case @graingert to make it clear.

I feel like this helper is making the assertion harder to reason about. Inlining getattr(source, attr) calls would be easier to read IMO.

it gets very long winded when you add getattr(source, attr) eveywhere

What's the rationale for changing this test class name? It looks like unnecessary noise to me.

@adamchainz said to do it: #10033 (comment)

Ditto about the class __name__ change.

__name__?

I'd say on Python < 3.6

done

You'll want to branch off < 3.6.

that's how this works

Might make sense to check explicitly set name too, because '__name' obviously will not work.

?

class A:
    __print = cached_property(print, '__print')

This will not work and we can easily detect it too.

What should the condition be?

@sir-sigurd can you advise about the changes to address your comments?

I'll push my edits tomorrow.

Why the warning, but not an exception?

Semver

I also still don't understand why it's useful to allow writing code that doesn't work.

well it does work, it just behaves as though @cached_property were not there. And it writes to some self.__ attr

I think silently failing to cache the property should be considered not working.

I would consider that as not working

What should the exception be?

ValueError maybe since passing along name can fix it?

Might make sense to raise exception in this case:

class Test:
    @cached_property
    def a(self):
        pass
    b = a

Yep