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

implement basic ding-dong bot #39

Merged
merged 7 commits into from
May 9, 2020
Merged

implement basic ding-dong bot #39

merged 7 commits into from
May 9, 2020

Conversation

wj-Mcat
Copy link
Collaborator

@wj-Mcat wj-Mcat commented Apr 30, 2020

Implement python-wechaty message module, so it can receive and send message.

run example/ding-dong-bot.py, bot can say dong when it receive ding in personal chat, not in a Room.

@wj-Mcat wj-Mcat requested a review from a team as a code owner April 30, 2020 07:22
@wj-Mcat wj-Mcat requested a review from huan April 30, 2020 07:31
@wj-Mcat
Copy link
Collaborator Author

wj-Mcat commented Apr 30, 2020

This is the simplest version of python-wechaty. There are some problems:

  • chatie_grpc: I generate the grpc code locally and the genreated code can't pass code linting tools.
  • mypy: missing-import error, need to be fixed.

@huan
Copy link
Member

huan commented Apr 30, 2020

That's a HUGE PR! Thank you so much for the efforts for pushing our ding-dong-bot.py moving forward!

I'm working on it now, and I hope I can give you some good feedback later.

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.

Hi @wj-Mcat ,

You did a great job of connecting the Wechaty with the GRPC service, I believe we are very close to the runnable ding-dong-bot.py!

This PR is a quite long one which includes lots of codes and API changes, I have some comments/suggestions/thoughts and have put some of them in the comments.

Please read them carefully and let me know what you think by reply to my comments under this PR so that we can start a conversation to make it clearer.

I believe there will be quite a number of questions that need to be discussed between us after you reply to my comments, so I'd like to suggest that we can schedule a short zoom meeting to have a discussion on it in person if you think it's necessary too.

requirements.txt Show resolved Hide resolved
src/wechaty_puppet/file_box.py Outdated Show resolved Hide resolved
src/wechaty_puppet/message.py Outdated Show resolved Hide resolved
src/wechaty/user/image.py Outdated Show resolved Hide resolved
src/wechaty/user/mini_program.py Outdated Show resolved Hide resolved
src/wechaty/user/message.py Outdated Show resolved Hide resolved
src/wechaty/user/room.py Outdated Show resolved Hide resolved
src/wechaty/user/room.py Outdated Show resolved Hide resolved
src/wechaty/user/room_invitation.py Outdated Show resolved Hide resolved
src/wechaty/user/room_invitation.py Outdated Show resolved Hide resolved
@wj-Mcat
Copy link
Collaborator Author

wj-Mcat commented Apr 30, 2020

@huan I think it's necessary. see you tonight.

@huan
Copy link
Member

huan commented Apr 30, 2020

Some links that we have talked about tonight:

  1. Understanding the Wechaty Architecture for Hostie: go-wechaty implementation wechaty-puppet-hostie discussion. go-wechaty#22

image

  1. The Puppet Server Architecture: https://github.com/wechaty/python-wechaty#python-wechaty-developing-plan

image

  1. Node NPM FileBox Project: https://github.com/huan/file-box
  2. Node NPM MemoryCard Project: https://github.com/huan/memory-card
  3. PyPI(pip) packages to be published: PyPI(pip) packages to be published #43
  4. Generate Puppet Schema from TypeScript Automatically: Generate Puppet Schema from TypeScript Automatically go-wechaty#26

image

@@ -72,6 +72,21 @@ def __init__(self, contact_id: str):
# self.name: str = "default_acontact"
self.payload: Optional[ContactPayload] = None

_event_stream: AsyncIOEventEmitter = AsyncIOEventEmitter()
Copy link
Member

Choose a reason for hiding this comment

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

Are there any blockers that prevent we inherited Contact from the EventEmitter class?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

At the beginning, I think that Contact/Room class should focus on logic processing. One module should be as simple as possible. If we add EventEmit ability to them, they look redundant. Although, I have not split the code. Besides,Event listener should be global function, not a member of a class.

For example, room-invite event should be a moduler function, don't couple with Contact/Room class.

# user/room.py
class Room:
    pass

# user/room_events.py
def listen_room_invite(room: Room, inviter: Contact, ):
    # do some logic processing

But, If we put all things which is about room toRoom class, that also looks good to me. If I makeRoom inherited from AsyncIOEventEmitter , there is only a few code changes. So, it's simple to refactor code.

What do you think about it.

Copy link
Member

Choose a reason for hiding this comment

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

ic. Let's talk about it in today's meeting.


# puppet_options = PuppetOptions(token='your-token-here')
hostie_puppet = HostiePuppet(PuppetOptions('your-token-here'), 'hostie-puppet')
bot = Wechaty(hostie_puppet).on('message', message)
Copy link
Member

@huan huan May 9, 2020

Choose a reason for hiding this comment

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

Can we use the same argument as the TypeScript Wechaty?

bot = Wechaty({
  puppet: 'wechaty-puppet-hostie', 
  puppetOptions: { 
    token: 'the_token',
  },
})

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Ok, I will refactor it later.

log.info("can't load contact %s payload, message : %s",
self.name,
str(e.args))
if force_sync or self.is_ready():
Copy link
Member

@huan huan May 9, 2020

Choose a reason for hiding this comment

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

I believe what you want to do is:

-  if force_sync or self.is_ready():
+  if force_sync or !self.is_ready():

Am I right?

And I'd like to suggest to following the following flow logic:

if self.is_ready() and not force_sync:
  return

// Do the sync staffs

Because the above flow logic can save us for indent for the following codes.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

I find this bug just now. I have changed it.
Thank you for giving me so detailed code view.

return []
log.info('load contact tags for %s', self)
tag_ids = await self.puppet.tag_contact_list(self.contact_id)
tags = [self.wechaty.Tag.load(contact_id)
Copy link
Member

Choose a reason for hiding this comment

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

I don't think the Tag.load() can use a contact_id as it's argument.

self._payload = await self.puppet.friendship_payload(
self.friendship_id)
if not self.is_ready():
friendship_search_response = await self.puppet.friendship_payload(
Copy link
Member

Choose a reason for hiding this comment

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

Why the return data from puppet.friendship_payload() need to be converted again?

I believe it should be already the payload interface that we need already?

cls,
# need return type annotation
query: Union[str, MessageUserQueryFilter]):
async def find(cls, talker_id: Optional[str] = None,
Copy link
Member

Choose a reason for hiding this comment

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

A QueryFilter would be better for this argument.

@@ -381,10 +385,10 @@ async def mention_self(self) -> bool:
Check if a message is mention self.
:return:
"""
self_id = self.puppet.self_id()
self_id = self.wechaty.contact_id
Copy link
Member

Choose a reason for hiding this comment

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

I saw you have use xxx.get_wechaty() before, however, I'd like to prefer xxx.wechaty.yyy more because it looks clearer.

Please be consistent.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

This module is designed very early. When I translate ts to python, I find a problem: python has no static property. So, we can't set inner static member as typescript. https://github.com/wechaty/wechaty/blob/master/src/accessory.ts#L30

If we keep consistent, xxx.wechaty is a static member, rather than a property which get/set inner static member.

Copy link
Member

Choose a reason for hiding this comment

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

I'm ok with either design of them.

Let's keep trying our best to keep consistent.

from dataclasses import dataclass


class MessageType(Enum):
Copy link
Member

Choose a reason for hiding this comment

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

Cloud we import all those constant values from the chatie-grpc module? Because that module is auto-generated from the chatie/grpc golden of the source repo.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

wechaty_puppet is a abstract module which should not contains any thing in wechaty_puppet_** .

There will be many wechaty_puppet_**, which MessageType member value may be different. eg:

# wechaty_puppet_hostie
class MessageType(Enum):
    Contact = 0
    Text = 1

# wechaty_puppet_padplus
class MessageType(Enum):
    Contact = 1
    Text = 2

So, How to resolve different wechaty_puppet_** Enum constant value same as wechaty_puppet.
I think keep that using names to ensure consistency is a good solution:

# wechaty_puppet_chatie
MESSAGE_TYPE_MAP = {
  ChatieMessageType.Contact: MessageType.Contact
}

# wechaty_puppet_padplus
MESSAGE_TYPE_MAP = {
  PadPlusMessageType.Contact: MessageType.Contact
}

So, use name-convention to keep same information.

How do think?

limitations under the License.
"""

VERSION = '0.0.1'
Copy link
Member

Choose a reason for hiding this comment

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

please change to 0.0.0.

It should be auto-generated via the GitHub Actions before be published/deployed.

huan
huan previously approved these changes May 9, 2020
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.

Great progress!

LGTM.

@huan
Copy link
Member

huan commented May 9, 2020

This is a HUGE Pull Request!

I'd like to suggest that we can merge this PR first, and then we can submit smaller PRs to continue working on our project.

Please let me know if you want to wait this PR to grow larger instead of merging it right now...

@wj-Mcat
Copy link
Collaborator Author

wj-Mcat commented May 9, 2020

Yes, I think we should merge this pr, and split modules to me and @huangaszaq . So, we can push progress to be faster.

@huan huan merged commit af7f9f0 into wechaty:master May 9, 2020
@huan
Copy link
Member

huan commented May 9, 2020

Great to know that you agree with me to merge it!

Please use separate PR by creating different branches for different feature developments/bug fixes, because we can easily discuss different topics with focus.

Thank you for the great effort to push our Python Wechaty project moving forward, keep fighting!

@huangaszaq
Copy link
Contributor

Six six six! LGTM! I have been studying ts these days, and may be ready to work on it.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants