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

Add doc #1458

Merged
merged 24 commits into from
Jul 12, 2018
Merged

Add doc #1458

merged 24 commits into from
Jul 12, 2018

Conversation

lijiarui
Copy link
Member

@lijiarui lijiarui commented Jul 10, 2018

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.

Please remove the changes of file docs/index.md, and follow my reviews.

@@ -479,8 +509,12 @@ export class Contact extends Accessory implements Sayable {

/**
* Contact gender
* > Tips: ContactGender is enum here. </br>
* - ContactGender.Unknown = 0</br>
Copy link
Member

Choose a reason for hiding this comment

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

Please remove all numbers in the ENUM type.

ContactGender.Unknown is enough, the user should care about the number value about it, and also should never use the magic number.

* @example
* const bot = new Wechaty()
* bot.on('friendship', async friendship => {
* console.log(`received friend event from ${friendship.contact().name()}`)
Copy link
Member

Choose a reason for hiding this comment

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

Please make the expression shorter, which is better for readability.

  const contact = friendship.contact()
  const name = contact.name()
  console.log(`received friend event from ${name}`)

/**
* Return the Friendship Type
* > Tips: FriendshipType is enum here. </br>
* - FriendshipType.Unknown = 0 </br>
Copy link
Member

Choose a reason for hiding this comment

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

Get rid of all magic numbers

* # 3. send Contact
*
* if (/^lijiarui$/i.test(m.text())) {
* const contactCard = await bot.Contact.find({name: 'lijiarui'})

This comment was marked as resolved.

/**
* Get the type from the message.
* > Tips: MessageType is Enum here. </br>
* - MessageType.Unknown = 0, </br>
Copy link
Member

Choose a reason for hiding this comment

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

Get rid of all magic numbers.

* @param {string} [text] If set this para, it will change room announce.
* @returns {(Promise<void | string>)}
*
* @example <caption>When you say anything in a room, it will get room announce. </caption>

This comment was marked as resolved.

src/user/room.ts Outdated
* @deprecated use sync() instead
* @returns {Promise<void>}
* @description
* Force reload data for Room, use {@link Room#sync} instead
Copy link
Member

Choose a reason for hiding this comment

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

should use @ignore here.

So does sync() because they are not the public API

Copy link
Member Author

Choose a reason for hiding this comment

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

Why we do not make this API public?

* @example
* import { Wechaty } from 'wechaty'
* See more:
* - [What is a Puppet in Wechaty](https://github.com/Chatie/wechaty-getting-started/wiki/FAQ-EN#31-what-is-a-puppet-in-wechaty)
Copy link
Member

Choose a reason for hiding this comment

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

Please do not use Wiki at wechaty-getting-started repository, we have to keep that repo as simple as possible.

I'll disable the wiki of it later, please move the FAQ to the Wiki at Wechaty Repository.

English version should be named with FAQ, and the ZH version you can name it.

src/wechaty.ts Outdated
*
* # 2 TypeScript
Copy link
Member

Choose a reason for hiding this comment

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

Remove the TypeScript part, because it has no typing at all, it's just ES6.

src/wechaty.ts Outdated
* @example
* const bot = new Wechaty()
* await bot.start()
*
Copy link
Member

Choose a reason for hiding this comment

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

// after logged in

@huan
Copy link
Member

huan commented Jul 11, 2018

Please put the following introduction to the doc as a start point.

Message.age()

Returns the message age in seconds.

For example, the message is sent at time 8:43:01, and when we received it in Wechaty, the time is 8:43:15, then the age() will return 8:43:15 - 8:43:01 = 14 (seconds)

Message.toFileBox()

Extract the Media File from the Message, and put it into the FileBox.

Room.qrcode()

Get QR Code of the Room from the room, which can be used as scan and join the room.

@huan
Copy link
Member

huan commented Jul 11, 2018

Did you follow all my reviews? If not, which one had been skipped, and why?

Please leave message by replying the PR and let me know what you did in your lastest commits.

@lijiarui
Copy link
Member Author

lijiarui commented Jul 11, 2018

Yes, I follow all your reviews and change all as you request besides the following 3 things:

  1. not totally agree with @ignore part as you said here

  2. here you said add a link to wechaty wiki. But I have the following questions:
    I agree with this suggestion but doesn't think clearly about how to add, because you just delete this and I don't know when you delete this. Then if I add something like this to the wiki, if anyone deletes this it may cause link error. But I didn't find a good solution to this.

  3. here you said I should move wechaty-getting-started wiki to wechaty wiki, but I want to classify the wiki faq as what I did in wechaty-getting-started, and show as doc faq showed. But I don't know whether it is allowed to classify at wechaty wiki. Maybe will better, maybe will more confuse, not very sure.

@huan
Copy link
Member

huan commented Jul 11, 2018

No, you missed some of the reviews, like remove the docs/index.md at #1458 (review)

@lijiarui
Copy link
Member Author

Yes, you are right. I agree with you.

After we finish the 3 discussions, I will remove the docs/index.md because the latest online doc depends on this branch file.

@huan
Copy link
Member

huan commented Jul 11, 2018

No need to discuss anymore.

Please just remove that file and I'll merge it.

@lijiarui
Copy link
Member Author

I remove that file and you can merge it.

@huan huan merged commit 02d7036 into wechaty:master Jul 12, 2018
@huan
Copy link
Member

huan commented Jul 12, 2018

Thank you very much for the great JSDoc!

@lijiarui lijiarui deleted the add-doc branch July 30, 2018 15:39
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.

None yet

2 participants