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 wechaty document #725

Merged
merged 20 commits into from Aug 31, 2017

Conversation

@lijiarui
Member

lijiarui commented Aug 11, 2017

@zixia

This comment has been minimized.

Show comment
Hide comment
@zixia

zixia Aug 12, 2017

Member

Thanks! Please get at least three votes(approved) from our contributor team first, because the document needs to be improved heavily and continuously.

Member

zixia commented Aug 12, 2017

Thanks! Please get at least three votes(approved) from our contributor team first, because the document needs to be improved heavily and continuously.

@zixia

Thanks for all those hard works for the documentation, I believe it will help other developers a lot after published.

There have two additional jobs of this PR:

  1. Please also generate the MarkDown file for the jsdoc by run npm run doc and make sure the generated document file at docs/index.md is what we want.
  2. Please add @private to all the methods/functions which are not listed at the README API Reference section because they are just for internal use and should not be treat as published API

I also post all my reviews to the PR, please follow them as well.

Show outdated Hide outdated src/room.ts Outdated
Show outdated Hide outdated src/room.ts Outdated
Show outdated Hide outdated src/room.ts Outdated
Show outdated Hide outdated src/room.ts Outdated
Show outdated Hide outdated src/room.ts Outdated
Show outdated Hide outdated src/room.ts Outdated
Show outdated Hide outdated src/room.ts Outdated
Show outdated Hide outdated src/room.ts Outdated
Show outdated Hide outdated src/room.ts Outdated
Show outdated Hide outdated src/room.ts Outdated

@zixia zixia removed the request for review from imWildCat Aug 16, 2017

lijiarui added some commits Aug 17, 2017

@lijiarui

This comment has been minimized.

Show comment
Hide comment
@lijiarui

lijiarui Aug 17, 2017

Member

here is the auto generate doc: https://github.com/lijiarui/wechaty/blob/new-doc/docs/index.md

not sure why it has double wechaty intro....

will keep on dig in it, or anyone can help? @ax4

Member

lijiarui commented Aug 17, 2017

here is the auto generate doc: https://github.com/lijiarui/wechaty/blob/new-doc/docs/index.md

not sure why it has double wechaty intro....

will keep on dig in it, or anyone can help? @ax4

Show outdated Hide outdated src/message.ts Outdated
@kis87988

This comment has been minimized.

Show comment
Hide comment
@kis87988

kis87988 Aug 18, 2017

Contributor

maybe we should add all return promise type with notice "await"

Contributor

kis87988 commented Aug 18, 2017

maybe we should add all return promise type with notice "await"

@zixia

This comment has been minimized.

Show comment
Hide comment
@zixia

zixia Aug 18, 2017

Member

In our current implementation of the doc generator script npm run doc, when we are generating the document, it is actually reading the jsdoc from dist/**/*.js, like dist/src/wechaty.js. You can generate the files under dist/ directory by run npm run dist, and make sure all the javascript class with jsdoc looks good.

BTW: I saw lots of ISC License in the generated document, it's not right. The license of the Wechaty project is Apache-2.0. There might have some comments say "ISC license", should be replaced.

Member

zixia commented Aug 18, 2017

In our current implementation of the doc generator script npm run doc, when we are generating the document, it is actually reading the jsdoc from dist/**/*.js, like dist/src/wechaty.js. You can generate the files under dist/ directory by run npm run dist, and make sure all the javascript class with jsdoc looks good.

BTW: I saw lots of ISC License in the generated document, it's not right. The license of the Wechaty project is Apache-2.0. There might have some comments say "ISC license", should be replaced.

@zixia zixia requested review from hczhcz and mukaiu Aug 18, 2017

@kis87988

This comment has been minimized.

Show comment
Hide comment
@kis87988

kis87988 Aug 18, 2017

Contributor

ISC License should be changed in whchaty.ts @line#68

Contributor

kis87988 commented Aug 18, 2017

ISC License should be changed in whchaty.ts @line#68

@zixia

This comment has been minimized.

Show comment
Hide comment
@zixia

zixia Aug 18, 2017

Member

Thanks, @kis87988, I had fixed that by removing the LICENSE info.

Member

zixia commented Aug 18, 2017

Thanks, @kis87988, I had fixed that by removing the LICENSE info.

@lijiarui lijiarui requested a review from zixia Aug 18, 2017

@coveralls

This comment has been minimized.

Show comment
Hide comment
@coveralls

coveralls Aug 18, 2017

Coverage Status

Coverage remained the same at 54.688% when pulling 5a7802b on lijiarui:master into d3aa251 on Chatie:master.

coveralls commented Aug 18, 2017

Coverage Status

Coverage remained the same at 54.688% when pulling 5a7802b on lijiarui:master into d3aa251 on Chatie:master.

@zixia

Thanks for the update, it looks better now.

Could you please include the docs/index.md in the next PR?

Please make sure that the index.md file content is expected.

Have a nice weekend!

Show outdated Hide outdated src/contact.ts Outdated
Show outdated Hide outdated src/message.ts Outdated
Show outdated Hide outdated src/message.ts Outdated
Show outdated Hide outdated src/message.ts Outdated
Show outdated Hide outdated src/message.ts Outdated
Show outdated Hide outdated src/message.ts Outdated
Show outdated Hide outdated src/message.ts Outdated
Show outdated Hide outdated src/message.ts Outdated
Show outdated Hide outdated src/room.ts Outdated
Show outdated Hide outdated src/wechaty.ts Outdated

lijiarui added some commits Aug 20, 2017

lijiarui added some commits Aug 20, 2017

@lijiarui lijiarui requested a review from zixia Aug 20, 2017

lijiarui added some commits Aug 21, 2017

@zixia

There are too many unrelated changes in the diff.

Please only keep the related change in PR.

Show outdated Hide outdated docs/index.md Outdated
Show outdated Hide outdated src/contact.ts Outdated
Show outdated Hide outdated src/message.ts Outdated
Show outdated Hide outdated src/message.ts Outdated

lijiarui added some commits Aug 21, 2017

@lijiarui lijiarui requested a review from zixia Aug 21, 2017

@zixia

The jsdoc looks better to me now, thank you very much for all the work of writing the documents and examples!

I have some minor reviews. After resolved them, I'll be able to vote for my approvement.

Show outdated Hide outdated docs/index.md Outdated
* @example
* ```ts

This comment has been minimized.

@zixia

zixia Aug 21, 2017

Member

Could please confirm it for me that we do not need "```ts" block inside @example?

@zixia

zixia Aug 21, 2017

Member

Could please confirm it for me that we do not need "```ts" block inside @example?

This comment has been minimized.

@lijiarui

lijiarui Aug 21, 2017

Member

yes, I looked into the doc and confirmed, then changed all ``` inside @example

see http://usejsdoc.org/tags-example.html

@lijiarui

lijiarui Aug 21, 2017

Member

yes, I looked into the doc and confirmed, then changed all ``` inside @example

see http://usejsdoc.org/tags-example.html

This comment has been minimized.

@zixia

zixia Aug 21, 2017

Member

Great, thanks.

@zixia

zixia Aug 21, 2017

Member

Great, thanks.

@@ -277,52 +278,40 @@ export class Contact implements Sayable {
/**
* Contact gender
*
* @returns Gender.Male(2) | Gender.Female(1) | Gender.Unknown(0)

This comment has been minimized.

@zixia

zixia Aug 21, 2017

Member

Just use Gender.Male Gender.Female and Gender.Unknown, there's no need to refer the number value of them because that's why ENUM type exists: Do not rely on the magic numbers.

So does the other ENUM types.

@zixia

zixia Aug 21, 2017

Member

Just use Gender.Male Gender.Female and Gender.Unknown, there's no need to refer the number value of them because that's why ENUM type exists: Do not rely on the magic numbers.

So does the other ENUM types.

This comment has been minimized.

@lijiarui

lijiarui Aug 21, 2017

Member

well, m.type() return too much type, are you sure use like this?

https://github.com/lijiarui/wechaty/blob/master/docs/index.md#Message+type

@lijiarui

lijiarui Aug 21, 2017

Member

well, m.type() return too much type, are you sure use like this?

https://github.com/lijiarui/wechaty/blob/master/docs/index.md#Message+type

This comment has been minimized.

@zixia

zixia Aug 21, 2017

Member

Yes. Just use the ENUM names please.

@zixia

zixia Aug 21, 2017

Member

Yes. Just use the ENUM names please.

Show outdated Hide outdated src/contact.ts Outdated
Show outdated Hide outdated src/contact.ts Outdated
Show outdated Hide outdated src/message.ts Outdated
Show outdated Hide outdated src/message.ts Outdated
Show outdated Hide outdated src/room.ts Outdated

@lijiarui lijiarui requested a review from zixia Aug 27, 2017

@zixia

zixia approved these changes Aug 27, 2017

@zixia zixia referenced this pull request Aug 30, 2017

Closed

William doc dev #701

@hczhcz

hczhcz approved these changes Aug 30, 2017 edited

Great job! The documentation looks good to me.

There might be something need to be modified, but I would suggest merge this PR and do these updates in successor PRs.

* Wechaty - https://github.com/chatie/wechaty
*
* Copyright 2016-2017 Huan LI <zixia@zixia.net>
* @copyright 2016-2017 Huan LI <zixia@zixia.net>

This comment has been minimized.

@hczhcz

hczhcz Aug 30, 2017

Member

It would be better if all files share the same header (copyright info, license, etc)

@hczhcz

hczhcz Aug 30, 2017

Member

It would be better if all files share the same header (copyright info, license, etc)

This comment has been minimized.

@zixia

zixia Aug 31, 2017

Member

@hczhcz thanks! I'll do that later by an util script written by me at https://github.com/Chatie/wechaty/blob/master/script/update-license.ts, in case you are interested at.

@zixia

zixia Aug 31, 2017

Member

@hczhcz thanks! I'll do that later by an util script written by me at https://github.com/Chatie/wechaty/blob/master/script/update-license.ts, in case you are interested at.

@binsee

This comment has been minimized.

Show comment
Hide comment
@binsee

binsee Aug 31, 2017

Member

look forward to merge

Member

binsee commented Aug 31, 2017

look forward to merge

@lijiarui

This comment has been minimized.

Show comment
Hide comment
@lijiarui

lijiarui Aug 31, 2017

Member

@binsee Could you help to approve this pull request? then @zixia can merge this, thanks!

Member

lijiarui commented Aug 31, 2017

@binsee Could you help to approve this pull request? then @zixia can merge this, thanks!

@lijiarui lijiarui requested a review from binsee Aug 31, 2017

@binsee

binsee approved these changes Aug 31, 2017

good job

@zixia

This comment has been minimized.

Show comment
Hide comment
@zixia

zixia Aug 31, 2017

Member

Great! Thank you @hczhcz @binsee for reviewing this PR, appreciate that!

Member

zixia commented Aug 31, 2017

Great! Thank you @hczhcz @binsee for reviewing this PR, appreciate that!

@zixia zixia merged commit da8c652 into Chatie:master Aug 31, 2017

4 of 6 checks passed

continuous-integration/appveyor/pr AppVeyor build failed
Details
continuous-integration/travis-ci/pr The Travis CI build failed
Details
ci/circleci Your tests passed on CircleCI!
Details
codacy/pr Good work! A positive pull request.
Details
codeclimate All good!
Details
security/snyk No new vulnerabilities
Details
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment