Suggest to refactor the `ready`, `payload` function of the related entity classes

[fish-ball]

When I fixed the bug on the Friendship class (within PR #96 ), I found the old code is not clear, and causes some error.

class Friendship(Accessory, Acceptable):

    def __init__(self, friendship_id: str):
        """
        initialization constructor for friendship
        """
        self.friendship_id = friendship_id
        self._payload: Optional[FriendshipPayload] = None

        log.info('Friendship constructor %s', friendship_id)

        if self.__class__ is Friendship:
            raise Exception(
                'Friendship class can not be instanciated directly!')
        if self.puppet is None:
            raise Exception(
                'Friendship class can not be instanciated without a puppet!')

    @classmethod
    def load(cls, friendship_id: str) -> Friendship:
        """
        load friendship without payload, which loads in a lazy way
        :param friendship_id:
        :return: initialized friendship
        """
        return cls(friendship_id)

    @property
    def payload(self) -> FriendshipPayload:
        """
        get the FriendShipPayload as a property
        :return:
        """
        if self._payload is None:
            self.ready()
        if self._payload is None:
            raise Exception('can"t load friendship payload')
        return self._payload

    def is_ready(self) -> bool:
        """
        check if friendship is ready
        """
        return self.puppet is None or self.payload is None

    async def ready(self):
        """
        load friendship payload
        """
        if not self.is_ready():
            friendship_search_response = await self.puppet.friendship_payload(
                friendship_id=self.friendship_id)
            self._payload = friendship_search_response
        if self.payload is None:
            raise Exception('can"t not load friendship payload %s'
                            % self.friendship_id)

[fish-ball]

The procedure which uses this class is inside the function Wechaty.init_puppet_event_bridge:

1. in friendship_listener, we call self.Friendship.load(payload.friendship_id) to instanciate a Friendship object, now the newly created Friendship object has no _payload data.
2. then we call await friendship.ready(), which trigger the lazy load to fetch the Friendship.payload from server.
3. emit the fully loaded Friendship object by the friendship signal, so that the on methods registered can get the fully loaded Friendship objects as an argument.

[fish-ball]

The outer procedure is well, but inside the Friendship class there are some issues.

For the first one:

The @property payload is not garanteed to be not None, so the old implementation intent to call ready() first to ensure its existance.

    @property
    def payload(self) -> FriendshipPayload:
        """
        get the FriendShipPayload as a property
        :return:
        """
        if self._payload is None:
            self.ready()
        if self._payload is None:
            raise Exception('can"t load friendship payload')
        return self._payload

But obviously the @property payload function is not async, neither does it awaits the ready() function, and it causes an infinite loop when triggered in my test.

So I changed this function in my PR:

    @property
    def payload(self) -> FriendshipPayload:
        """
        get the FriendShipPayload as a property
        :return:
        """
        if self._payload is None:
            self.ready()
        if self._payload is None:
            raise Exception('can"t load friendship payload')
        return self._payload

What I've changed is I made it only returns the _payload object, and the return value is changed to Optional, not garanteed to be not null.

So comes the problem, when I submit the PR, the integration test is faild.

For example, for the following line:

        contact = self.wechaty.Contact.load(self.payload.contact_id)

I tells that the self payload is Nullable, so self.payload.contact_id do not pass the test.

[fish-ball]

In order to make it pass, I have to change the line as below:

        if not self.payload:
            raise Exception('payload not ready ...')
        contact = self.wechaty.Contact.load(self.payload.contact_id)

Adding a verbosing case detection to check its existance, just in order to get across the typing test.

So why we need a _payload and payload duplicate property? Type checking is not the python way.

So I think all the ready, is_ready, payload, _payload should be restructured all together.

[fish-ball]

In fact, we want all actions which requires _payload exists should work on a payload which has a type garanteed to be not None.

So every time we access the payload, we just lazy load it, like:

    @property
    async def payload(self) -> FriendshipPayload:
        """
        get the FriendShipPayload as a property
        :return:
        """
        if not self._payload:
            self._payload = await self.puppet.friendship_payload(
                friendship_id=self.friendship_id)
        return self._payload

Then we do not need async ready() and is_ready() any more.

When we want to use the payload (garanteed to be not None), or we want to preload the payload, we just await it, for the code mentioned above

        if not self.payload:
            raise Exception('payload not ready ...')
        contact = self.wechaty.Contact.load(self.payload.contact_id)

we can rewrite it as below:

        payload = await self.payload
        contact = self.wechaty.Contact.load(payload.contact_id)

The code is prettier, and no type error anymore.

[fish-ball]

So after all, @wj-Mcat, I want to know whether this pattern is a good practise, if it do, we can refactor all classes with payload in this way. Or further, we can move all payload proccess to an abstract base class.