-
-
Notifications
You must be signed in to change notification settings - Fork 2.6k
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
add room-leave event #370
add room-leave event #370
Conversation
Sorry, i cannot merge this because your PR didn't pass any of CI test. |
Yes, it cannot be merge now, because I should fix #364 first. And I have fix room.member(), please help to merge it first, thanks |
Sorry, I have no time to help you about this recently. You need to do it by yourself. |
Changes Unknown when pulling f0c8176 on lijiarui:room_leave into ** on Chatie:master**. |
@zixia maybe it has done... |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Please think of member()
method more.
src/puppet-web/firer.ts
Outdated
let leaverContact: Contact | null, removerContact: Contact | null | ||
if (leaver === this.userId) { | ||
leaverContact = Contact.load(this.userId) | ||
removerContact = room.member({alias: remover}) || room.member({name: remover}) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Please use the formal query filter name, instead of using an alias.
removerContact = room.member({roomAlias: remover}) || room.member({name: remover})
I will more prefer to write this piece of code in this way:
removerContact = room.member(remover)
Because Room.member()
should do the job for translate the string name to a Contact inside a room.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think using room.member()
is not strict here, because roomAlias
(alias) here is useless, bot just recognize contactAlias
and name
, if we use roomAlias
, then when someone's roomAlias is the same as other's name in the room. it will get the contact we don't mean go get.
So, maybe the new change I commit is the better way. we should may the code easier for developer, but more strict here.
Changes Unknown when pulling 77076c7 on lijiarui:room_leave into ** on Chatie:master**. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Please think deeper about the purpose of member()
method.
src/puppet-web/firer.ts
Outdated
} | ||
} else { | ||
removerContact = Contact.load(this.userId) | ||
leaverContact = room.member({contactAlias: remover}) || room.member({name: leaver}) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think it is better to use member(name)
here for better encapsulation.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Ok, I believe it will works.
Please get 2 more approving from other contributors as well, then I could be able to merge this. Thanks! |
@lijiarui This is the way of my favorite for using https://github.com/Chatie/wechaty/blob/master/src/puppet-web/firer.ts#L319 |
Changes Unknown when pulling 3e29a82 on lijiarui:room_leave into ** on Chatie:master**. |
1 similar comment
Changes Unknown when pulling 3e29a82 on lijiarui:room_leave into ** on Chatie:master**. |
The shorter, the better. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
it' good for me
Thanks all! I can merge this PR now, Cheers! |
fix #250