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.

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

feat: change say() method response type from void to Message #1866

Merged
merged 14 commits into from
Oct 31, 2019
Merged

feat: change say() method response type from void to Message #1866

merged 14 commits into from
Oct 31, 2019

Conversation

su-chang
Copy link
Member

I'm submitting a...

[ ] Bug Fix
[ ] 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

related issue: #1857

windmemory
windmemory previously approved these changes Oct 23, 2019
View reviewed changes
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.

Looks good to me, approved.

@huan huan requested a review from windmemory October 31, 2019 00:30
@huan
Copy link
Member

huan commented Oct 31, 2019

@windmemory Please only approve the PR that can be passed the CI tests.

It will not be good progress that we approve of the future work before we can make sure the current work can be done without any problem.

This PR can not pass the CI before the new version of wechaty-puppet had published the related new features, and the wechaty-puppet is not ready yet. So please only approve this PR after we had everything ready for it.

Thank you very much!

@windmemory
Copy link
Member

@huan got it
:P

@windmemory
Copy link
Member

Let's make the codeclimate happy first.

huan
huan requested changes Oct 31, 2019
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.

The codeclimate can be ignored, I'll remove it later.

Please follow my reviews.

src/types.ts Outdated Show resolved Hide resolved
src/types.ts Outdated Show resolved Hide resolved
huan
huan requested changes Oct 31, 2019
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.

Please check it more strictly.

src/user/contact.ts Outdated Show resolved Hide resolved
@su-chang su-chang requested a review from huan October 31, 2019 08:30
windmemory
windmemory previously approved these changes Oct 31, 2019
View reviewed changes
huan
huan requested changes Oct 31, 2019
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.

Good job!

One more thing: we should update the documentation as well.

Please update both the README.md and the JS Doc, after that, we will be able to merge this PR.

Cheers!

@huan
Copy link
Member

huan commented Oct 31, 2019

BTW: It would be better if you can remove the code of typeof msgId === 'string' because it's useless: the Typing system will be able to take care of that case.

@su-chang
Copy link
Member Author 

@param {(string | Contact | FileBox | UrlLink | MiniProgram)} something
send text, Contact, or file to contact. </br>
You can use {@link https://www.npmjs.com/package/file-box|FileBox} to send file
@returns {Promise<void | Message>}

import { FileBox }  from 'file-box'
const fileBox1 = FileBox.fromUrl('https://chatie.io/wechaty/images/bot-qr-code.png')
const fileBox2 = FileBox.fromFile('/tmp/text.txt')
await contact.say(fileBox1)
const msg1 = await contact.say(fileBox1) // only supported by puppet-padplus
await contact.say(fileBox2)
const msg2 = await contact.say(fileBox2) // only supported by puppet-padplus

Should I add JS doc like this way?

Or declare only supported by puppet-padplus only once in head line?

@param {(string | Contact | FileBox | UrlLink | MiniProgram)} something
send text, Contact, or file to contact. </br>
You can use {@link https://www.npmjs.com/package/file-box|FileBox} to send file
@returns {Promise<void | Message>} Message type only supported by puppet-padplus.

import { FileBox }  from 'file-box'
const fileBox1 = FileBox.fromUrl('https://chatie.io/wechaty/images/bot-qr-code.png')
const fileBox2 = FileBox.fromFile('/tmp/text.txt')
await contact.say(fileBox1)
const msg1 = await contact.say(fileBox1)
await contact.say(fileBox2)
const msg2 = await contact.say(fileBox2)

Which way is better? Thanks.
@huan

@huan
Copy link
Member

huan commented Oct 31, 2019

Please just pick the one which you think is the best. Thanks.

@huan huan requested a review from windmemory October 31, 2019 09:54
windmemory
windmemory requested changes Oct 31, 2019
View reviewed changes
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.

Minor suggested changes.

README.md Outdated Show resolved Hide resolved
README.md Outdated Show resolved Hide resolved
README.md Outdated Show resolved Hide resolved
@windmemory
Copy link
Member

Looks good to me.

@huan huan merged commit df0aec4 into wechaty:master Oct 31, 2019
@huan
Copy link
Member

huan commented Oct 31, 2019

Thank you very much for the great improvement, Cheers!

su-chang added a commit to su-chang/wechaty that referenced this pull request Nov 1, 2019
@su-chang
Merge pull request #5 from Chatie/master
58fc1e1 
feat: change say() method response type from void to Message (wechaty#1866)
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

@windmemory windmemory windmemory 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.
3 participants
@su-chang @huan @windmemory