Skip to content

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

Merged
merged 11 commits into from
Jan 8, 2020
Merged

Add reason for logout event #1900

merged 11 commits into from
Jan 8, 2020

Conversation

su-chang
Copy link
Member

@su-chang su-chang commented Jan 6, 2020

I'm submitting a...

[ ] Bug Fix
[x] Feature
[ ] Other (Refactoring, Added tests, Documentation, ...)

Checklist

  • Commit Messages follow the Conventional Commits pattern
    • A feature commit message is prefixed "feat:"
    • A bugfix commit message is prefixed "fix:"
  • Tests for the changes have been added

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?

[ ] Yes
[ ] No

@su-chang
Copy link
Member Author

su-chang commented Jan 6, 2020

Related issue: wechaty/wechaty-puppet-padplus#100

@huan huan requested a review from windmemory January 6, 2020 14:53
Copy link
Member

@windmemory windmemory left a 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.

@su-chang
Copy link
Member Author

su-chang commented Jan 7, 2020

I think optional is flexible, if puppet which do not support the logout reason, it could ignore the parameter reason.

@windmemory
Copy link
Member

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 login and logout event since there is no reason for login event right now.

@su-chang
Copy link
Member Author

su-chang commented Jan 7, 2020

Got it.

windmemory
windmemory previously approved these changes Jan 7, 2020
@huan huan requested a review from windmemory January 7, 2020 13:05
@huan
Copy link
Member

huan commented Jan 7, 2020

@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!

@su-chang
Copy link
Member Author

su-chang commented Jan 8, 2020

@huan ping

@huan huan merged commit fcec6e7 into wechaty:master Jan 8, 2020
@huan
Copy link
Member

huan commented Jan 8, 2020

Thanks for making the CI green for this PR.

su-chang added a commit to su-chang/wechaty that referenced this pull request Jan 8, 2020
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