-
-
Notifications
You must be signed in to change notification settings - Fork 2.7k
Add reason for logout event #1900
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
Conversation
feat: change say() method response type from void to Message (wechaty#1866)
Merge wechaty/wechaty
Merge wechaty/wechaty
Merge wechaty master
Related issue: wechaty/wechaty-puppet-padplus#100 |
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 would prefer to have a separate logout event declaration here, because I think the reason should not be optional, but required.
I think optional is flexible, if puppet which do not support the logout reason, it could ignore the parameter |
If no reason, an empty string could also work. But optional also works. I will not insist on this point. But I would insist on the separation of |
Got it. |
@windmemory We should not approve any PR that did not pass the CI. If the CI is not green, we should never approve it unless we have to. Thanks! |
@huan ping |
Thanks for making the CI green for this PR. |
Add reason for logout event (wechaty#1900)
I'm submitting a...
Checklist
Description
please describe the changes that you are making
for features, please describe how to use the new feature
please include a reference to an existing issue, if applicable
Does this PR introduce a breaking change?