Skip to content

we need to shield wechaty_puppet"s **QueryFilter to user in wechaty #27

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.

Sign up for GitHub

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

Jump to bottom
Merged
merged 3 commits into from
Mar 22, 2020
Merged

we need to shield wechaty_puppet"s **QueryFilter to user in wechaty #27

merged 3 commits into from
Mar 22, 2020

Conversation

wj-Mcat
Copy link
Collaborator

@wj-Mcat wj-Mcat commented Mar 17, 2020

Problem: When developer use wechaty to query room/contact/friendship, they will use **QueryFilter from wechaty_puppet, so they should import wechaty_puppet too.

There is only one module that build a wrapper to QueryFilter class in ts wechaty . It's Message module that use MessageUserQueryFilter, so developer don't known wechaty_puppet MessageQueryFilter.

But I think MessageUserFilter is ugly. MessageQueryFilter is great and can be use in wechaty also.

Solution: Below is my simple code design.

if TYPE_CHECKING:
    from wechaty_puppet import (
        FileBox,
        # So that RoomQueryFilter can be use in python-wechaty also
        RoomQueryFilter as PuppetRoomQueryFilter
    )

class RoomQueryFilter:
    """
    python-wechaty room query filter
    """
    def transfer(self) -> PuppetRoomQueryFilter:
        """
        # convert python-wechaty RoomQueryFilter to PuppetRoomQueryFilter
        """
        return PuppetRoomQueryFilter()

class Room(Accessory, Sayable):
    @classmethod
    async def find_all(cls, query: RoomQueryFilter = None) -> List[Room]:
        """
        find room by query filter
        """
        puppet_query = query.transfer() if query is not None else None
        room_ids = await cls.get_puppet().room_search(puppet_query)
        # do something

@wj-Mcat
Copy link
Collaborator Author

wj-Mcat commented Mar 17, 2020

The topic is about building query wrappers for wechaty_puppet **QueryFilter class.
I check the code in ts-wechaty :

They all use QueryFilter from wechaty_puppt.

@huan huan mentioned this pull request Mar 19, 2020
Closed
@huan
Copy link
Member

huan commented Mar 19, 2020

If I understand you right, you are talking about there's an interface named MessageUserQueryFilter which you believe should be removed for clearness.

I think to remove it would be a good idea after reading the code you mentioned, so I made a breaking change to the Wechaty at https://github.com/wechaty/wechaty/pull/1931/files

Please let me know this is what you want, or feel free to correct me if I understand wrong.

Thank you very much.

@wj-Mcat
Copy link
Collaborator Author

wj-Mcat commented Mar 20, 2020

I think MessageQueryFilter interface can't be removed.

I think we should not use MessageQueryFilter as a func parameter in python-wechaty. If so, user-client-code should import MessageQueryFilter class from wechaty-puppet. Therefores developer, who use python-wechaty, will come into contact with two packages: python-wechaty and wechaty_puppet.

If we create a interface MessageQueryFilter in python-wechaty, developer only need to known python-wechaty.

Keep it as simple as possible for developer.

  +--------------------------+ +--------------------------+
  |                    user-client-code                   |
  +--------------------------+ +--------------------------+
  +--------------------------+ +--------------------------+
  |                    python-wechaty                     |
  +--------------------------+ +--------------------------+
  +--------------------------+ +--------------------------+
  |                    wechaty-puppet                     |
  +--------------------------+ +--------------------------+

@huan
Copy link
Member

huan commented Mar 20, 2020

I understand your concern. It's very similar to wechaty/wechaty#1914: we have some problems with the NPM packages like this one:

user client code needs to import FileBox, so we re-export the FileBox from the Wechaty module for convenience.

In this case, I believe there's a very easy solution as well:

We can re-export the MessageQueryFilter from the python-wechaty so that the developer who uses python-wechaty will not need to contact any packages other than the python-wechaty

Please feel free to let me know what you think about it, thanks.

@wj-Mcat
Copy link
Collaborator Author

wj-Mcat commented Mar 20, 2020

Your solution is great.
We import MessageQueryFilter from wechaty_puppet to python-wechaty, so developer can import it in python-wechaty. Is that right?

@huan
Copy link
Member

huan commented Mar 20, 2020

Yes, it is.

Glad to know you like this solution!

@huan
Copy link
Member

huan commented Mar 20, 2020

So let's start discussing my PR wechaty/wechaty#1931

Do you agree that we should remove the MessageUserQueryFilter in wechaty' so that we can always use XXXQueryFilterfromwechaty-puppet`?

Please feel free to let me know what you think, thank you very much.

@wj-Mcat
Copy link
Collaborator Author

wj-Mcat commented Mar 20, 2020

I agree.
MessageUserQueryFilter is a copy/warpper of MessageQueryFilter. we can set MessageQueryFilter to all in python-wechaty modules , so that developer can use it from python-wechaty.

@huan
Copy link
Member

huan commented Mar 20, 2020 •
edited
Loading

Great!

Cloud you help me to approve that PR in Wechaty repository? Because that would help us remember this discussion together.

I'll merge that PR after you approved it.

Link to: wechaty/wechaty#1929

huan
huan approved these changes Mar 22, 2020
View reviewed changes
Copy link
Member

@huan huan left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM

@huan huan merged commit 26dc534 into wechaty:master Mar 22, 2020
@huan
Copy link
Member

huan commented Mar 22, 2020

Let's merge this PR first.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@huan huan huan approved these changes

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
2 participants
@wj-Mcat @huan